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

worker,etw: only enable ETW on the main thread #25907

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 3, 2019

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.

(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), or vcbuild test (Windows) passes (probably)
  • commit message follows commit guidelines

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
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 3, 2019
@addaleax addaleax added windows Issues and PRs related to the Windows platform. worker Issues and PRs related to Worker support. labels Feb 3, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20553/ (:heavy_check_mark:)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2019
@gireeshpunathil
Copy link
Member

@addaleax - if the events are registered only by main thread (though this change) then the ETW event callbacks (etw_events_enable_callback) are also received in the main thread? If so, do we need another level of callback through the async (etw_events_change_async) ? can't we directly invoke v8 from the win32 callback itself?

@addaleax
Copy link
Member Author

addaleax commented Feb 4, 2019

@gireeshpunathil I am unfamiliar with this code; however:

// Call v8 to enable or disable code event callbacks.
// Must be on default thread to do this.
// Note: It is possible to call v8 from ETW thread, but then
// event callbacks are received in the same thread. Attempts
// to write ETW events in this thread will fail.

(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.)

@addaleax
Copy link
Member Author

addaleax commented Feb 5, 2019

Landed in 63ab542

@addaleax addaleax closed this Feb 5, 2019
@addaleax addaleax deleted the worker-no-etw branch February 5, 2019 17:06
addaleax added a commit that referenced this pull request Feb 5, 2019
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 <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Feb 6, 2019
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 <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. worker Issues and PRs related to Worker support.
Projects
None yet
7 participants