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

src: force gc for flaky (deadlock) tests #57476

Closed
wants to merge 1 commit into from

Conversation

jakecastelli
Copy link
Member

@jakecastelli jakecastelli commented Mar 15, 2025

This patch adds a new flag force_gc_for_test (internal use for test purpose only) to force GC for flaky tests that suffer from deadlock. Refs: #54918 and #56827 (comment)

The deadlock occurs when (correct me if I am wrong):

  1. Main thread calls DrainTasks and executes worker tasks
  2. Worker task needs memory allocation requiring GC
  3. GC must run on main thread, but main thread is busy running the worker task

Notes:

  1. v8 RequestGarbageCollectionForTesting will require --expose-gc flag to work which means for the flaky tests that suffer from deadlock we will need // Flags: --expose-gc --force-gc-for-test at the top of the test file.
  2. This patch does not resolve the deadlock issue but rather an attempt to reduce the flaky tests without marking them as flaky.
  3. Based on the findings, I will take time looking into how chromium handles it and see if we can properly fix it in node, but any help would be greatly appreciated 🙏
  4. Bad at naming, if this patch is acceptable, please suggest a good name for the option name.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 15, 2025
@jakecastelli jakecastelli force-pushed the deflake-with-force-gc branch 3 times, most recently from b510857 to 9f8370d Compare March 15, 2025 01:51
@jakecastelli jakecastelli changed the title src,cli,test: force gc upon exit for flaky (deadlock) tests src,cli: force gc for flaky (deadlock) tests Mar 15, 2025
@jakecastelli jakecastelli changed the title src,cli: force gc for flaky (deadlock) tests src: force gc for flaky (deadlock) tests Mar 15, 2025
@jakecastelli jakecastelli force-pushed the deflake-with-force-gc branch from 9f8370d to f8fb19d Compare March 15, 2025 01:52
This patch adds a new flag `force_gc_for_test` (internal use for test
purpose only) to force GC specifically for flaky tests that suffer from
deadlock.

The deadlock occurs when:
1. Main thread calls DrainTasks and executes worker tasks
2. Worker task needs memory allocation requiring GC
3. GC must run on main thread, but main thread is busy running the
   worker task
@jakecastelli jakecastelli force-pushed the deflake-with-force-gc branch from f8fb19d to 43781fb Compare March 15, 2025 01:56
@jakecastelli jakecastelli requested a review from lpinca March 15, 2025 14:14
@lpinca
Copy link
Member

lpinca commented Mar 15, 2025

v8 RequestGarbageCollectionForTesting will require --expose-gc flag to work which means for the flaky tests that suffer from deadlock we will need // Flags: --expose-gc --force-gc-for-test at the top of the test file.

If we need to update tests one by one we don't need a new flag. We can just call globalThis.gc({ type: 'major' }) when the test is done.

@lpinca
Copy link
Member

lpinca commented Mar 15, 2025

I also fear that if we did this for all tests we would end up ignoring the underlying issue because it would no longer be a problem. So far only a handful of people have investigated the issue and apparently it is not trivial (#56827). When we no longer have a constant reminder that it exists via CI, it will be forgotten.

@jakecastelli
Copy link
Member Author

Fair points, I will continue to dig the underlining issue with the rest of us, and hopefully we can get it properly fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants