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

Improve detectOpenHandles #13417

Merged
merged 10 commits into from
Sep 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap
Original file line number Diff line number Diff line change
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:

● TCPSERVERWRAP
DNSCHANNEL,TCPSERVERWRAP

12 | const app = new Server();
13 |
Comment on lines 19 to 25
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 modified the logic here and now it will report all types at the same stack.
Not sure if the prompt message needs to be modified, but I'm not good at English and can't do anything about it.🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Awesome change! We could probably land this on main?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do this, but the tests in the main branch don't reflect this. I couldn't think of a test example like this for a while. (DNSCHANNEL is ignored in main branch)

Expand Down
5 changes: 4 additions & 1 deletion packages/jest-core/src/__tests__/collectHandles.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,12 @@ describe('collectHandles', () => {
it('should not collect the DNSCHANNEL open handle', async () => {
const handleCollector = collectHandles();

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

// We must drop references to it
resolver = null;

const openHandles = await handleCollector();

expect(openHandles).not.toContainEqual(
Expand Down
84 changes: 45 additions & 39 deletions packages/jest-core/src/collectHandles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,7 @@ export default function collectHandles(): HandleCollectionResult {
// 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 === 'DNSCHANNEL' ||
type === 'ZLIB' ||
type === 'SIGNREQUEST'
) {
if (type === 'PROMISE') {
Comment on lines 85 to +86
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only TIMERWRAP was added before jest repo was migrated to ts, it's a bit troublesome to find commit records, I don't have confirmation.
For other types, I have checked the submitted PRs and tests, and they are theoretically safe.
Anyway, this is in a major release and can be safely tested in a beta release.

Copy link
Member

Choose a reason for hiding this comment

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

include PerformanceObserver to fix CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try to destroy it. #13417 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Original issue with a reproduction: #11051

If node doesn't require a disconnect to exit cleanly, we shouldn't report it

Copy link
Member

Choose a reason for hiding this comment

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

We can probably add

const client = require("prom-client");
client.collectDefaultMetrics();

as an integration test

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'm not really sure what detectOpenHandles is for now, technically most of the object types removed by this PR should not block process exit, but it seems that some users use this to detect memory leaks/object leaks. Would love to hear your opinion.

Copy link
Member

Choose a reason for hiding this comment

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

It's used to detect handles keeping the process running after tests complete (such as a rogue server, setInterval (without unref() or clearInterval)) etc.. It shouldn't be used for memory leaks per se (although open handles indicate a missing cleanup of some sorts), but since it forces GC runs it oftens helps with memory usage

Copy link
Member

Choose a reason for hiding this comment

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

essentially to find out why https://jestjs.io/docs/cli#--forceexit would be needed

return;
}
const error = new ErrorWithStack(type, initHook, 100);
Expand Down Expand Up @@ -141,14 +132,18 @@ export default function collectHandles(): HandleCollectionResult {
// For example, Node.js TCP Servers are not destroyed until *after* their
// `close` callback runs. If someone finishes a test from the `close`
// callback, we will not yet have seen the resource be destroyed here.
await asyncSleep(100);
await asyncSleep(0);

if (activeHandles.size > 0) {
// For some special objects such as `TLSWRAP`.
// Ref: https://github.com/facebook/jest/issues/11665
runGC();
await asyncSleep(30);
Copy link
Contributor Author

@liuxingbaoyu liuxingbaoyu Oct 10, 2022

Choose a reason for hiding this comment

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

Checking length is free, let's check it first. Another 100ms might be a bit long? I modified it to 30ms, you can also modify it to 0-10ms if you want, lol.

Copy link
Member

Choose a reason for hiding this comment

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

as low as possible while still passing all tests (without introducing flake) seems ideal 😀


await asyncSleep(0);
if (activeHandles.size > 0) {
// For some special objects such as `TLSWRAP`.
// Ref: https://github.com/facebook/jest/issues/11665
runGC();

await asyncSleep(0);
}
}

hook.disable();
Expand All @@ -167,33 +162,44 @@ export function formatHandleErrors(
errors: Array<Error>,
config: Config.ProjectConfig,
): Array<string> {
const stacks = new Set();

return (
errors
.map(err =>
formatExecError(err, config, {noStackTrace: false}, undefined, true),
)
// E.g. timeouts might give multiple traces to the same line of code
// This hairy filtering tries to remove entries with duplicate stack traces
.filter(handle => {
const ansiFree: string = stripAnsi(handle);

const match = ansiFree.match(/\s+at(.*)/);

if (!match || match.length < 2) {
return true;
}
const stacks = new Map<string, {stack: string; names: Set<string>}>();

errors.forEach(err => {
const formatted = formatExecError(
err,
config,
{noStackTrace: false},
undefined,
true,
);

const stack = ansiFree.substr(ansiFree.indexOf(match[1])).trim();
// E.g. timeouts might give multiple traces to the same line of code
// This hairy filtering tries to remove entries with duplicate stack traces

if (stacks.has(stack)) {
return false;
}
const ansiFree: string = stripAnsi(formatted);
const match = ansiFree.match(/\s+at(.*)/);
if (!match || match.length < 2) {
return;
}

stacks.add(stack);
const stackText = ansiFree.substr(ansiFree.indexOf(match[1])).trim();

const name = ansiFree.match(/(?<=● {2}).*$/m);
if (!name?.length) {
return;
}

const stack = stacks.get(stackText) || {
names: new Set(),
stack: formatted.replace(name[0], '%%OBJECT_NAME%%'),
};

stack.names.add(name[0]);

stacks.set(stackText, stack);
});

return true;
})
return Array.from(stacks.values()).map(({stack, names}) =>
stack.replace('%%OBJECT_NAME%%', Array.from(names).join(',')),
);
}