fix: improve open handles detection #9532

SimenB · 2020-02-07T11:06:38Z

Summary

Our current implementation is often fooled by promises, and we lose track of stack traces across async boundaries, meaning https://github.com/facebook/jest/blob/481077b09b05dc659b1fbd9ecf34a6486425208d/packages/jest-core/src/collectHandles.ts#L14 returns false negatives.

This is a work project before and after this fix

I honestly don't know about the first one (it's a false positive), but the second one lead to us fixing upstream here: valery-barysok/session-file-store#84

Fixes #8554.

Test plan

This breaks the stack trace of the existing server test. We get our current trace from the GETADDRINFOREQWRAP event, but that's gone in favour of TCPSERVERWRAP by the time the test exits, which does not have a stack trace. Unsure how to best deal with it

thymikee · 2020-02-07T14:34:54Z

Looks like this causes a regression. Mind taking a look?

SimenB · 2020-02-07T14:43:56Z

Yeah, I mentioned it in the OP. Unsure how to deal with it

SimenB · 2020-02-19T13:09:11Z

I think it might be best to keep the ones that are deleted, but mark them as closed - that way if some async thing that async started something that's hanging but itself is closed (like the server example that CI caught) would still be visible, although it'd be noisier in the output

e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap

SimenB · 2020-02-20T10:07:42Z

e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap

@@ -1,5 +1,26 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`deals with http servers and promises 1`] = `
+Jest has detected the following 1 open handle potentially preventing Jest from exiting:
+Of them 1 was collected within 100ms of the tests completing.


this probably isn't clear enough...

How about:

Detected 3 open handles potentially preventing Jest from exiting: 1. <red>DNSCHANNEL (uncollected, potential leak) 2. <red>Timeout (uncollected, potential leak) 3. <yellow>TCPSERVERWRAP (collected 100ms after test run, potentially spawned other uncollected handles)</yellow> 4 | const server = http.createServer(); 5 | await new Promise(resolve => { > 6 | server.listen(resolve); [optionally] There were no uncollected handles - this is unexpected if your tests do not exit cleanly.

Not sure about the "optional" part. Shouldn't it say something like "Collected handles shouldn't prevent Jest from closing. Try something else"?

thoughts? I kept the heading, since if there were many collected, it looked really noisy...

I pushed that, since it's an improvement regardless

What do you think about moving the "Of them 1 was collected..." below, so it separates collected from uncollected more obvoiusly?

Detected 6 open handles potentially preventing Jest from exiting: ... 1 of them was collected within 100ms after the test run completed: ...

I'm also not a fan of:

Jest has detected the following 1 open handle potentially preventing Jest from exiting

as it contains "Jest" twice.
This is shorter and gets to the point:

Detected 6 open handles potentially preventing Jest from exiting

packages/jest-core/src/cli/index.ts

SimenB · 2020-02-20T10:16:41Z

My work project now looks like this, btw (the zlibs were there before as well):

So we keep the serverwrap from supertest that was previously removed, but we mark it as "collected". I think that makes sense 🙂 Hopefully people will look at the uncollected ones with stack trace first.

thymikee · 2020-02-20T11:14:43Z

packages/jest-core/src/runJest.ts

  onComplete?: (result: AggregatedResult) => void;
  outputStream: NodeJS.WriteStream;
 };

-const processResults = (
+const wait = promisify(setTimeout);


can we move this into createOpenHandlesResult so it doesn't affect all test runs?

affect how? this just creates a new function in the module scope

ah, CI complains. Doesn't locally though...

This is likely not an issue at al. The point is, if we can avoid calling something, for a regular user in this scenario, why not doing so. Even if it's a small thing, they like to add up.

SimenB · 2020-02-20T11:29:28Z

Hmm, weird CI sees a bunch if stuff I don't see locally...

jmikrut · 2020-04-16T13:26:01Z

I would love to see this merged. Anything I can do to help here?

SimenB · 2020-04-16T13:38:30Z

@jmikrut would love help making CI green 🙂 Once green this is good to land. On CI there are different timers than on my local machine, so hard to make the tests stable.

I can rebase this in the meantime

SimenB · 2020-08-07T22:36:59Z

--runInBand in the tests seems to give same result locally as on CI - might help investigate at some point

github-actions · 2021-06-20T00:04:18Z

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.

SimenB requested review from thymikee and jeysal February 7, 2020 11:06

facebook-github-bot added the cla signed label Feb 7, 2020

SimenB force-pushed the improve-open-handle-detection branch from 300d5cc to c9c10e9 Compare February 10, 2020 12:58

SimenB mentioned this pull request Feb 19, 2020

--detectOpenHandles not showing any message even test not finished completely. #6937

Closed

SimenB force-pushed the improve-open-handle-detection branch from c9c10e9 to 9ade171 Compare February 20, 2020 09:56

thymikee reviewed Feb 20, 2020

View reviewed changes

e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap Show resolved Hide resolved

SimenB force-pushed the improve-open-handle-detection branch from 9ade171 to 01c19f5 Compare February 20, 2020 10:06

SimenB commented Feb 20, 2020

View reviewed changes

packages/jest-core/src/cli/index.ts Show resolved Hide resolved

thymikee reviewed Feb 20, 2020

View reviewed changes

SimenB force-pushed the improve-open-handle-detection branch from 6e27752 to a0500ca Compare February 20, 2020 12:05

SimenB force-pushed the improve-open-handle-detection branch from c1e8f0e to a58d262 Compare April 16, 2020 13:40

SimenB force-pushed the improve-open-handle-detection branch from a58d262 to dc23efd Compare August 6, 2020 12:28

SimenB force-pushed the improve-open-handle-detection branch from dc23efd to 0ee0a52 Compare August 7, 2020 22:37

SimenB added 7 commits August 23, 2020 23:14

fix: improve open handles detection

edf09ee

show both collected and uncollected handles

bdceca5

copyright header

8b27298

update snap after copyright

4fd7321

rollback unneeded message-util changes

210cbb6

ditch headings

3cd7ecd

linefeed tweaks

648ba9b

SimenB added 5 commits August 23, 2020 23:14

keep message-util change

2d64ef3

remove one more internal thing

c85f4eb

no promisify

35428db

maybe remove another internal from ci

5aab3b7

maybe?

935035a

SimenB force-pushed the improve-open-handle-detection branch from 0ee0a52 to 935035a Compare August 23, 2020 21:15

mkcode mentioned this pull request Sep 30, 2020

Improve UX for hanging tests MLH-Fellowship/jest#36

Closed

Mr0grog mentioned this pull request May 20, 2021

Wait for closed resources to actually close before detecting open handles #11429

Merged

SimenB closed this in #11429 May 20, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2021

SimenB deleted the improve-open-handle-detection branch April 25, 2023 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve open handles detection #9532

fix: improve open handles detection #9532

SimenB commented Feb 7, 2020

thymikee commented Feb 7, 2020

SimenB commented Feb 7, 2020

SimenB commented Feb 19, 2020 •

edited

SimenB Feb 20, 2020

thymikee Feb 20, 2020

SimenB Feb 20, 2020

SimenB Feb 20, 2020

SimenB Feb 20, 2020

thymikee Feb 20, 2020

thymikee Feb 20, 2020

SimenB commented Feb 20, 2020

thymikee Feb 20, 2020

SimenB Feb 20, 2020

SimenB Feb 20, 2020

thymikee Feb 20, 2020

SimenB commented Feb 20, 2020

jmikrut commented Apr 16, 2020

SimenB commented Apr 16, 2020 •

edited

SimenB commented Aug 7, 2020

github-actions bot commented Jun 20, 2021

fix: improve open handles detection #9532

fix: improve open handles detection #9532

Conversation

SimenB commented Feb 7, 2020

Summary

Test plan

thymikee commented Feb 7, 2020

SimenB commented Feb 7, 2020

SimenB commented Feb 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Feb 20, 2020

jmikrut commented Apr 16, 2020

SimenB commented Apr 16, 2020 • edited

SimenB commented Aug 7, 2020

github-actions bot commented Jun 20, 2021

SimenB commented Feb 19, 2020 •

edited

SimenB commented Apr 16, 2020 •

edited