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: move JoinThread() back into exit callback #31468

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

worker: move JoinThread() back into exit callback

de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
Environment::RunAndClearNativeImmediates() comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from JoinThread() potentially
unhandled (and unintentionally silenced through the TryCatch).

This affected exceptions thrown from the 'exit' event of the
Worker, and made the parallel/test-worker-message-type-unknown
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the JoinThread() call back to where it was
before.

Refs: #31386

src: harden running native SetImmediate()s slightly

Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: nodejs#31386
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels Jan 22, 2020
@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 22, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 22, 2020

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

👍 this comment to approve fast-tracking in order to fix CI.

@addaleax addaleax mentioned this pull request Jan 22, 2020
4 tasks
@addaleax
Copy link
Member Author

Landed in 8fb5fe2...31b31a3

@addaleax addaleax closed this Jan 23, 2020
addaleax added a commit that referenced this pull request Jan 23, 2020
de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: #31386

PR-URL: #31468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax added a commit that referenced this pull request Jan 23, 2020
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

PR-URL: #31468
Refs: #31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax addaleax deleted the worker-exit-fix branch January 23, 2020 02:00
codebytere pushed a commit that referenced this pull request Feb 17, 2020
de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: #31386

PR-URL: #31468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

PR-URL: #31468
Refs: #31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
@codebytere
Copy link
Member

Blocked on backport of #31386 - if that's not intended to go back i can update the label!

MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: nodejs#31386

PR-URL: nodejs#31468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit to addaleax/node that referenced this pull request Apr 1, 2020
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

PR-URL: nodejs#31468
Refs: nodejs#31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: #31386

Backport-PR-URL: #32301
PR-URL: #31468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

Backport-PR-URL: #32301
PR-URL: #31468
Refs: #31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@codebytere codebytere mentioned this pull request Apr 3, 2020
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++. fast-track PRs that do not need to wait for 48 hours to land. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants