test: remove flakiness on macOS test #56971

anonrig · 2025-02-09T00:52:35Z

Locally I could reproduce this on macOS M4 machine using tools/test.py --repeat 10000 test/parallel/test-net-write-fully-async-hex-string.js after this change I couldn't reproduce it.

Ref: https://github.com/nodejs/node/actions/runs/13220154720/job/36904132463?pr=56970

codecov · 2025-02-09T02:15:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.12%. Comparing base (b181535) to head (a712b4f).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56971      +/-   ##
==========================================
- Coverage   89.13%   89.12%   -0.01%     
==========================================
  Files         665      665              
  Lines      193165   193179      +14     
  Branches    37191    37201      +10     
==========================================
- Hits       172181   172177       -4     
- Misses      13729    13739      +10     
- Partials     7255     7263       +8

see 41 files with indirect coverage changes

lpinca · 2025-02-09T07:14:14Z

I don't know if this invalidates the original intention of the test but it seems to also fix the issue on my machine. Can you do the same also for test-net-write-fully-async-buffer.js (see #52959 (comment))?

This might also help with #54918 because before this change when the test timed out the issue was the same:

$ lldb -p 53757
(lldb) process attach --pid 53757
Process 53757 stopped
* thread #1, name = 'MainThread', queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
    frame #0: 0x00007ff80d888aaa libsystem_kernel.dylib`__psynch_cvwait + 10
libsystem_kernel.dylib`__psynch_cvwait:
->  0x7ff80d888aaa <+10>: jae    0x7ff80d888ab4 ; <+20>
    0x7ff80d888aac <+12>: movq   %rax, %rdi
    0x7ff80d888aaf <+15>: jmp    0x7ff80d886662 ; cerror_nocancel
    0x7ff80d888ab4 <+20>: retq   
Target 0: (node) stopped.
Executable module set to "/Users/luigi/code/node/out/Release/node".
Architecture set to: x86_64-apple-macosx-.
(lldb) bt
* thread #1, name = 'MainThread', queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x00007ff80d888aaa libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007ff80d8c77a8 libsystem_pthread.dylib`_pthread_cond_wait + 1193
    frame #2: 0x00000001067b37a3 node`uv_cond_wait(cond=<unavailable>, mutex=<unavailable>) at thread.c:819:7 [opt]
    frame #3: 0x00000001058ccfdb node`node::NodePlatform::DrainTasks(v8::Isolate*) [inlined] node::LibuvMutexTraits::cond_wait(cond=0x00007fba92404cb8, mutex=0x00007fba92404c48) at node_mutex.h:175:5 [opt]
    frame #4: 0x00000001058ccfc1 node`node::NodePlatform::DrainTasks(v8::Isolate*) [inlined] node::ConditionVariableBase<node::LibuvMutexTraits>::Wait(this=0x00007fba92404cb8, scoped_lock=<unavailable>) at node_mutex.h:249:3 [opt]
    frame #5: 0x00000001058ccfc1 node`node::NodePlatform::DrainTasks(v8::Isolate*) [inlined] node::TaskQueue<v8::Task>::BlockingDrain(this=0x00007fba92404c48) at node_platform.cc:641:20 [opt]
    frame #6: 0x00000001058ccfab node`node::NodePlatform::DrainTasks(v8::Isolate*) [inlined] node::WorkerThreadsTaskRunner::BlockingDrain(this=0x00007fba92404c48) at node_platform.cc:215:25 [opt]
    frame #7: 0x00000001058ccfab node`node::NodePlatform::DrainTasks(this=0x0000600000c98000, isolate=<unavailable>) at node_platform.cc:468:33 [opt]
    frame #8: 0x0000000105732a60 node`node::SpinEventLoopInternal(env=0x00007fba95833600) at embed_helpers.cc:44:17 [opt]
    frame #9: 0x0000000105891e33 node`node::NodeMainInstance::Run() [inlined] node::NodeMainInstance::Run(this=<unavailable>, exit_code=<unavailable>, env=0x00007fba95833600) at node_main_instance.cc:111:9 [opt]
    frame #10: 0x0000000105891d99 node`node::NodeMainInstance::Run(this=<unavailable>) at node_main_instance.cc:100:3 [opt]
    frame #11: 0x00000001057f9d46 node`node::Start(int, char**) [inlined] node::StartInternal(argc=<unavailable>, argv=<unavailable>) at node.cc:1489:24 [opt]
    frame #12: 0x00000001057f9ba2 node`node::Start(argc=<unavailable>, argv=<unavailable>) at node.cc:1496:27 [opt]
    frame #13: 0x00007ff80d5382cd dyld`start + 1805

lpinca · 2025-02-09T07:37:14Z

It is already known that the main thread gets locked by the GC thread during DrainTasks (see #56827) so I think we should not do this as it is only a manual way to work around #54918, not a fix. I don't think it is feasible or correct to do this for all tests that fail due to #56827.

If the flakiness is unbearable I think it is better to revert #56365.

lpinca

See #56971 (comment)

anonrig · 2025-02-09T18:20:34Z

See #56971 (comment)

The goal is to make sure the drain is called on GC, and this doesn't break it and actually make sure that we have a major GC call. I don't think this changes the test, and regardless of fixing the flakiness, it actually makes it more correct IMHO. Since it is not the type of GC we validate but the side-effect it causes.

Can we find a middle ground and land this PR as it is? @lpinca I don't want to make it a flaky test and make it non-flaky upon the fix of the deadlock bug. I'd like to make the test more stable regardless of our internal bugs.

#56971 (comment)

lpinca · 2025-02-09T19:28:42Z

I've dismissed my "request changes". For consistency the same change should be applied to test-net-write-fully-async-buffer.js. I can't comment on the correctness as I don't know why type: minor was used.

This test is currently one of the best to reproduce #54918. I'll have to revert this change locally to see if #54918 persists when a fix is found.

anonrig · 2025-02-09T19:31:02Z

For consistency the same change should be applied to test-net-write-fully-async-buffer.js.

Sounds like a valid request to me: #56981

lpinca · 2025-02-09T19:35:17Z

Why not adding another commit here or changing both in the same commit? In this way the discussion about the change is in the same place for both tests.

lpinca · 2025-02-09T19:44:55Z

gc({ type: 'minor' }) was added in f5ce6b1.

cc: @tniessen

nodejs-github-bot · 2025-02-09T22:58:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/65103/

mcollina

lgtm

anonrig · 2025-02-11T15:14:02Z

@nodejs/tsc couldn't find any reviewers for this. would you mind reviewing to NOT wait for 4 more days?

nodejs-github-bot · 2025-02-11T15:24:26Z

Landed in 5dafb48...66549b4

PR-URL: #56971 Refs: https://github.com/nodejs/node/actions/runs/13220154720/job/36904132463?pr=56970 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>

test: remove flakiness on macOS test

ea8e6b9

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Feb 9, 2025

lpinca previously requested changes Feb 9, 2025

View reviewed changes

lpinca mentioned this pull request Feb 9, 2025

src: fix platform shutdown deadlock #56827

Open

anonrig mentioned this pull request Feb 9, 2025

test: reduce flakiness on test-net-write-fully-async-buffer #56981

Closed

anonrig requested a review from jasnell February 9, 2025 19:31

test: reduce flakiness on test-net-write-fully-async-buffer

a712b4f

anonrig requested a review from lpinca February 9, 2025 19:36

anonrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Feb 9, 2025

anonrig added the review wanted PRs that need reviews. label Feb 9, 2025

anonrig requested review from mcollina, ljharb and aduh95 February 10, 2025 02:55

mcollina approved these changes Feb 11, 2025

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 11, 2025

marco-ippolito approved these changes Feb 11, 2025

View reviewed changes

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 11, 2025

nodejs-github-bot closed this Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: remove flakiness on macOS test #56971

test: remove flakiness on macOS test #56971

anonrig commented Feb 9, 2025

codecov bot commented Feb 9, 2025 •

edited

Loading

lpinca commented Feb 9, 2025

lpinca commented Feb 9, 2025 •

edited

Loading

lpinca left a comment •

edited

Loading

anonrig commented Feb 9, 2025

lpinca commented Feb 9, 2025 •

edited

Loading

anonrig commented Feb 9, 2025

lpinca commented Feb 9, 2025

lpinca commented Feb 9, 2025

nodejs-github-bot commented Feb 9, 2025

mcollina left a comment

anonrig commented Feb 11, 2025

nodejs-github-bot commented Feb 11, 2025

test: remove flakiness on macOS test #56971

test: remove flakiness on macOS test #56971

Conversation

anonrig commented Feb 9, 2025

codecov bot commented Feb 9, 2025 • edited Loading

Codecov Report

lpinca commented Feb 9, 2025

lpinca commented Feb 9, 2025 • edited Loading

lpinca left a comment • edited Loading

Choose a reason for hiding this comment

anonrig commented Feb 9, 2025

lpinca commented Feb 9, 2025 • edited Loading

anonrig commented Feb 9, 2025

lpinca commented Feb 9, 2025

lpinca commented Feb 9, 2025

nodejs-github-bot commented Feb 9, 2025

mcollina left a comment

Choose a reason for hiding this comment

anonrig commented Feb 11, 2025

nodejs-github-bot commented Feb 11, 2025

codecov bot commented Feb 9, 2025 •

edited

Loading

lpinca commented Feb 9, 2025 •

edited

Loading

lpinca left a comment •

edited

Loading

lpinca commented Feb 9, 2025 •

edited

Loading