-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,12 +62,16 @@ export default function collectHandles(): HandleCollectionResult { | |
triggerAsyncId, | ||
resource: {} | NodeJS.Timeout, | ||
) { | ||
// Skip resources that should not generally prevent the process from | ||
// exiting, not last a meaningfully long time, or otherwise shouldn't be | ||
// tracked. | ||
if ( | ||
type === 'PROMISE' || | ||
type === 'TIMERWRAP' || | ||
type === 'ELDHISTOGRAM' || | ||
type === 'PerformanceObserver' || | ||
type === 'RANDOMBYTESREQUEST' | ||
type === 'RANDOMBYTESREQUEST' || | ||
type === 'DNSCHANNEL' | ||
Comment on lines
+73
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! 😀 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
) { | ||
return; | ||
} | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. Will play around with this and see how it looks or maybe tweak the formatting over the weekend. 👍