Improve `detectOpenHandles` by liuxingbaoyu · Pull Request #13417 · jestjs/jest

liuxingbaoyu · 2022-10-10T01:40:20Z

Summary

Removed some previously ignored detection types.

Test plan

CI GREEN

liuxingbaoyu · 2022-10-10T01:43:06Z

packages/jest-core/src/collectHandles.ts

      // tracked.
-      if (
-        type === 'PROMISE' ||
-        type === 'TIMERWRAP' ||
-        type === 'ELDHISTOGRAM' ||
-        type === 'PerformanceObserver' ||
-        type === 'RANDOMBYTESREQUEST' ||
-        type === 'DNSCHANNEL' ||
-        type === 'ZLIB' ||
-        type === 'SIGNREQUEST'
-      ) {
+      if (type === 'PROMISE') {


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.

include PerformanceObserver to fix CI?

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

Original issue with a reproduction: #11051

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

We can probably add

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

as an integration test

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.

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

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

liuxingbaoyu · 2022-10-10T01:44:34Z

e2e/__tests__/__snapshots__/detectOpenHandles.ts.snap

 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 |


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.🤦‍♂️

Awesome change! We could probably land this on main?

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)

liuxingbaoyu · 2022-10-10T01:46:11Z

packages/jest-core/src/collectHandles.ts

-    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);


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.

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

liuxingbaoyu · 2022-10-10T02:01:55Z

The failure in CI I should know how to fix, but I can't reproduce it locally (win) anyway, which is weird. Even I can't reproduce it using gitpod (linux). 😕

Seems to be related to node version, node18 on CI passes, node16 fails while failure is expected.

Yes, this seems to be an undocumented behavior, there is no such type PerformanceObserver in the docs for both node16 and node18.

legobeat · 2023-05-01T11:02:31Z

CI error seems resolved

SimenB · 2023-05-12T09:33:30Z

/easycla

linux-foundation-easycla · 2023-05-12T09:33:34Z

The committers listed above are authorized under a signed CLA.

✅ login: liuxingbaoyu (d1ab5c3, 04fd4ba, 5732328)
✅ login: SimenB / name: Simen Bekkhus (319180c, 751eec5, b0d7c51, f827305, 7e3584d, c518fae, e569539)

netlify · 2023-09-19T08:45:01Z

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`e569539`
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/6509c09d56b81c00089c9537
😎 Deploy Preview	https://deploy-preview-13417--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

SimenB · 2023-09-19T08:45:14Z

Hey @liuxingbaoyu, finally got back to this 😀 Could you sign the CLA? 🙂

liuxingbaoyu · 2023-09-19T11:51:27Z

Sorry for forgetting this!
Of course, it's done!

SimenB · 2023-09-19T11:52:28Z

Wonderful, thanks!

github-actions · 2023-10-20T00:08:19Z

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 · 2023-10-30T13:36:39Z

https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.1

improve

d1ab5c3

facebook-github-bot added the cla signed label Oct 10, 2022

liuxingbaoyu commented Oct 10, 2022

View reviewed changes

SimenB added this to the Jest 30 milestone Oct 10, 2022

liuxingbaoyu added 2 commits October 11, 2022 09:53

test ci

04fd4ba

fix test

5732328

SimenB added 3 commits September 19, 2023 10:47

Merge branch 'main' into improve-detect

319180c

changelog

751eec5

no deprecated method

b0d7c51

SimenB force-pushed the improve-detect branch from 9e51506 to b0d7c51 Compare September 19, 2023 08:47

SimenB enabled auto-merge (squash) September 19, 2023 08:48

Merge branch 'main' into improve-detect

f827305

SimenB disabled auto-merge September 19, 2023 11:52

SimenB enabled auto-merge (squash) September 19, 2023 11:52

SimenB changed the title ~~[jest-30] Improve detectOpenHandles~~ Improve detectOpenHandles Sep 19, 2023

SimenB added 3 commits September 19, 2023 14:33

Merge branch 'main' into improve-detect

7e3584d

Merge branch 'main' into improve-detect

c518fae

lint

e569539

SimenB disabled auto-merge September 19, 2023 19:57

SimenB merged commit 4727f80 into jestjs:main Sep 19, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2023

Uh oh!

Comments

Conversation

liuxingbaoyu commented Oct 10, 2022

Summary

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxingbaoyu Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxingbaoyu commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legobeat commented May 1, 2023

Uh oh!

SimenB commented May 12, 2023

Uh oh!

linux-foundation-easycla bot commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for jestjs ready!

Uh oh!

SimenB commented Sep 19, 2023

Uh oh!

liuxingbaoyu commented Sep 19, 2023

Uh oh!

SimenB commented Sep 19, 2023

Uh oh!

github-actions bot commented Oct 20, 2023

Uh oh!

SimenB commented Oct 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

liuxingbaoyu Oct 10, 2022 •

edited

Loading

liuxingbaoyu commented Oct 10, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 12, 2023 •

edited

Loading

netlify bot commented Sep 19, 2023 •

edited

Loading