★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25907Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@bnoordhuis bnoordhuis

@jasnell jasnell

@cjihrig cjihrig

@richardlau richardlau

@gireeshpunathil gireeshpunathil

Assignees
No one assigned
Labels
C++ author ready windows worker
Projects
None yet
Milestone
No milestone
7 participants
@addaleax @nodejs-github-bot @gireeshpunathil @bnoordhuis @jasnell @cjihrig @richardlau
@addaleax
Copy link
Member

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
@addaleax
worker,etw: only enable ETW on the main thread 
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
Loading status checks…
b8624ab
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the C++ label Feb 3, 2019

@addaleax addaleax added windows worker labels Feb 3, 2019

@addaleax addaleax requested a review from gireeshpunathil Feb 3, 2019

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 3, 2019 •
edited

CI: https://ci.nodejs.org/job/node-test-pull-request/20553/ (✔️)
@cjihrig

cjihrig approved these changes Feb 3, 2019
View changes

@bnoordhuis

bnoordhuis approved these changes Feb 3, 2019
View changes

@richardlau

richardlau approved these changes Feb 3, 2019
View changes

@addaleax addaleax added the author ready label Feb 3, 2019

@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 4, 2019 •
edited

@gireeshpunathil I am unfamiliar with this code; however:

node/src/node_win32_etw_provider.cc

Lines 121 to 125 in 5506dcd

// 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.)
@gireeshpunathil

gireeshpunathil approved these changes Feb 5, 2019
View changes

@jasnell

jasnell approved these changes Feb 5, 2019
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 5, 2019

Landed in 63ab542

@addaleax addaleax closed this Feb 5, 2019

@addaleax addaleax deleted the addaleax:worker-no-etw branch Feb 5, 2019

addaleax added a commit that referenced this pull request Feb 5, 2019

@addaleax
worker,etw: only enable ETW on the main thread 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
63ab542

@addaleax addaleax referenced this pull request Feb 5, 2019

Closed

Investigate flaky parallel/test-worker-memory #23277

addaleax added a commit that referenced this pull request Feb 6, 2019

@addaleax
worker,etw: only enable ETW on the main thread 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
c959d60

@targos targos referenced this pull request Feb 14, 2019

Merged

v11.10.0 proposal #26098

This was referenced Feb 15, 2019

Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.