Ignore DNSCHANNEL when using --detectOpenHandles #11470
Conversation
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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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. 👍
| type === 'RANDOMBYTESREQUEST' || | ||
| type === 'DNSCHANNEL' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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! 😀
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
The one failure in |
|
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. |
Summary
It turns out that Node.js only holds a weak reference on
DNSCHANNELasync resources, so they won't prevent a process from exiting. This changes--detectOpenHandlesto ignoreDNSCHANNELhandles. (Much credit to @joshkel for digging into the Node.js source on this.)This also solves a problem where messages about
DNSCHANNELcould 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 theDNSCHANNELresource 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).