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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,7 @@
### Fixes

- `[jest-circus, @jest/test-sequencer]` Remove dependency on `jest-runner` ([#11466](https://github.com/facebook/jest/pull/11466))
- `[jest-core]` Do not warn about `DNSCHANNEL` handles when using the `--detectOpenHandles` option ([#11470](https://github.com/facebook/jest/pull/11470))
- `[jest-runner]` Remove dependency on `jest-config` ([#11466](https://github.com/facebook/jest/pull/11466))
- `[jest-worker]` Loosen engine requirement to `>= 10.13.0` ([#11451](https://github.com/facebook/jest/pull/11451))

Expand Down
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap
Expand Up @@ -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. 👍


12 | const app = new Server();
13 |
Expand Down
14 changes: 14 additions & 0 deletions packages/jest-core/src/__tests__/collectHandles.test.js
Expand Up @@ -6,6 +6,7 @@
*
*/

import {promises as dns} from 'dns';
import http from 'http';
import {PerformanceObserver} from 'perf_hooks';
import collectHandles from '../collectHandles';
Expand Down Expand Up @@ -38,6 +39,19 @@ describe('collectHandles', () => {
obs.disconnect();
});

it('should not collect the DNSCHANNEL open handle', async () => {
const handleCollector = collectHandles();

const resolver = new dns.Resolver();
resolver.getServers();

const openHandles = await handleCollector();

expect(openHandles).not.toContainEqual(
expect.objectContaining({message: 'DNSCHANNEL'}),
);
});

it('should collect handles opened in test functions with `done` callbacks', done => {
const handleCollector = collectHandles();
const server = http.createServer((_, response) => response.end('ok'));
Expand Down
6 changes: 5 additions & 1 deletion packages/jest-core/src/collectHandles.ts
Expand Up @@ -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
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.

) {
return;
}
Expand Down