Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore DNSCHANNEL when using --detectOpenHandles #11470

Merged

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented May 28, 2021

Summary

It turns out that Node.js only holds a weak reference on DNSCHANNEL async resources, so they won't prevent a process from exiting. This changes --detectOpenHandles to ignore DNSCHANNEL handles. (Much credit to @joshkel for digging into the Node.js source on this.)

This also solves a problem where messages about DNSCHANNEL could obscure more meaningful messages about other handles that could be causing Jest to hang. Because Jest dedupes open handles by their stack trace, and async handles indirectly opened from other async handles get the same stack trace as the first handle, tracking the DNSCHANNEL resource can obscure other, more meaningful handles that were indirectly created and actually are causing the process to hang. Fixing this makes it a little easier for users to identify the actual issue that might be causing their tests to hang.

Fixes #9982.

Test plan

I added a unit test for this case and an end-to-end test to also covers it (but needed updating).

It turns out that Node.js only holds a weak reference on `DNSCHANNEL` async resources, so they won't prevent a process from exiting. This changes `--detectOpenHandles` to ignore `DNSCHANNEL` handles.

Because Jest dedupes open handles by their stack trace, and async handles indirectly opened from *other* async handles get the same stack trace as the first handle, tracking the `DNSCHANNEL` resource can obscure other, more meaningful handles that were indirectly created and actually *are* causing the process to hang. Fixing this makes it a little easier for users to identify the actual issue that might be causing their tests to hang.

Fixes jestjs#9982.
@@ -13,7 +13,7 @@ This usually means that there are asynchronous operations that weren't stopped i
exports[`prints out info about open handlers 1`] = `
Jest has detected the following 1 open handle potentially keeping Jest from exiting:

DNSCHANNEL
TCPSERVERWRAP
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually a perfect demonstration of the issue where meaningless DNSCHANNEL warnings could obscure the other indirectly created handles that are actually causing problems. 😄

I’m wondering if it might be a good follow-on PR to alter the stack traces for indirectly created async resources or the deduplication code slightly so that we log all the handles indirectly created from the same spot in user code, rather than just one.

Stack traces for indirectly created handles:
https://github.com/facebook/jest/blob/ae3fc943d48a8b690e7cd5872f652c6aaef91ddb/packages/jest-core/src/collectHandles.ts#L80-L86

Deduplicating based on stack trace:
https://github.com/facebook/jest/blob/ae3fc943d48a8b690e7cd5872f652c6aaef91ddb/packages/jest-core/src/collectHandles.ts#L151-L171

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks somewhat noisy with duplicated traces. But I guess we might miss out on the more "descriptive" handle? I'm open to a PR changing this with a good example of where it improves the output 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks somewhat noisy with duplicated traces.

For sure. Will play around with this and see how it looks or maybe tweak the formatting over the weekend. 👍

Comment on lines +73 to +74
type === 'RANDOMBYTESREQUEST' ||
type === 'DNSCHANNEL'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurred to me while adding this that the way we bail out early here can break the chain of indirect async resource tracking: https://github.com/facebook/jest/blob/ae3fc943d48a8b690e7cd5872f652c6aaef91ddb/packages/jest-core/src/collectHandles.ts#L80-L83

I don’t think it makes sense to change in this PR (it could be pretty high impact), but I wonder if we should not exit early here, but instead track these with isActive set to always return false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to a PR, but I don't really understand how it might break something? I'm sure I'll be convinced, tho! 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might just be being paranoid! My worry is that this will act too aggressively and cause us to warn on Jest internals or other things that should be ok. Will give this a try and see what happens.

@codecov-commenter
Copy link

Codecov Report

Merging #11470 (2c1d13e) into master (0088802) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11470   +/-   ##
=======================================
  Coverage   69.31%   69.31%           
=======================================
  Files         311      311           
  Lines       16293    16294    +1     
  Branches     4716     4717    +1     
=======================================
+ Hits        11293    11294    +1     
  Misses       4972     4972           
  Partials       28       28           
Impacted Files Coverage Δ
packages/jest-core/src/collectHandles.ts 70.14% <100.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0088802...2c1d13e. Read the comment docs.

@Mr0grog
Copy link
Contributor Author

Mr0grog commented May 28, 2021

The one failure in Node LTS on macOS-latest using jest-jasmine2 looks spurious; I’m not quite sure what’s happening there.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for yet another high quality PR! 🚀

The one failure in Node LTS on macOS-latest using jest-jasmine2 looks spurious; I’m not quite sure what’s happening there.

Yep, macos on CI sometimes just barfs

@SimenB SimenB merged commit 4cae680 into jestjs:master May 28, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The process exits normally, but I get an open handle error.
4 participants