-
Notifications
You must be signed in to change notification settings - Fork 30k
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
worker,etw: only enable ETW on the main thread #25907
Conversation
The Windows ETW code is not written to be compatible with multi-threading, and in particular it relies on global state like a single static `uv_async_t`. Adding that to multiple threads would corrupt the corresponding loops' handle queues. This addresses the flakiness of at least `test-worker-exit-code` and very likely other flaky tests that relate to Worker threads on Windows as well. Fixes: nodejs#25847 Fixes: nodejs#25702 Fixes: nodejs#24005 Fixes: nodejs#23873
CI: https://ci.nodejs.org/job/node-test-pull-request/20553/ (:heavy_check_mark:) |
@addaleax - if the events are registered only by main thread (though this change) then the |
@gireeshpunathil I am unfamiliar with this code; however: node/src/node_win32_etw_provider.cc Lines 121 to 125 in 5506dcd
(To add context: That comment was written in 2012. I would definitely take it with a grain of salt – if calling V8 APIs from a different thread is what’s happening, it’s likely to not be allowed by V8.) |
Landed in 63ab542 |
The Windows ETW code is not written to be compatible with multi threading, and in particular it relies on global state like a single static `uv_async_t`. Adding that to multiple threads would corrupt the corresponding loops' handle queues. This addresses the flakiness of at least `test-worker-exit-code` and very likely other flaky tests that relate to Worker threads on Windows as well. Fixes: #25847 Fixes: #25702 Fixes: #24005 Fixes: #23873 PR-URL: #25907 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>
The Windows ETW code is not written to be compatible with multi threading, and in particular it relies on global state like a single static `uv_async_t`. Adding that to multiple threads would corrupt the corresponding loops' handle queues. This addresses the flakiness of at least `test-worker-exit-code` and very likely other flaky tests that relate to Worker threads on Windows as well. Fixes: #25847 Fixes: #25702 Fixes: #24005 Fixes: #23873 PR-URL: #25907 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>
The Windows ETW code is not written to be compatible with multi-threading,
and in particular it relies on global state like a single static
uv_async_t
. Adding that to multiple threads would corrupt thecorresponding loops' handle queues.
This addresses the flakiness of at least
test-worker-exit-code
andvery likely other flaky tests that relate to Worker threads on Windows as well.
(I've marked the less-easy-to-reproduce flaky tests as likely fixed -- we can still re-open the issues if it turns they are still problematic.)
Fixes: #25847
Fixes: #25702 (likely)
Fixes: #24005 (likely)
Fixes: #23873 (likely)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes (probably)