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.

Sign up for GitHub

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

Jump to bottom

wasi,worker: handle termination exception #33386

Closed
wants to merge 2 commits into from
Closed

wasi,worker: handle termination exception #33386

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

Fixes: #33377

@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 May 13, 2020
addaleax
addaleax requested changes May 13, 2020
View reviewed changes
test/wasi/test-wasi-worker-terminate.js Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor

cjihrig commented May 16, 2020

@bnoordhuis are you still planning to work on this? I'm willing to implement #33386 (comment) if you'd like.

bnoordhuis added 2 commits May 16, 2020 11:52
@bnoordhuis
src: remove BeforeExit callback list
5660096 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.
@bnoordhuis
wasi,worker: handle termination exception
c46bbdd 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: nodejs#33377
bnoordhuis
bnoordhuis commented May 16, 2020
View reviewed changes
src/api/hooks.cc
@@ -30,15 +30,21 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) {
}

void EmitBeforeExit(Environment* env) {
env->RunBeforeExitCallbacks();
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"BeforeExit", env);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the lifetime of the scope a little but makes it more correct, IMO. In the old location it only covered part of the BeforeExit sequence.

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 20, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 20, 2020 •
edited by BridgeAR
Loading

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

@BridgeAR
Copy link
Member

BridgeAR commented May 23, 2020 •
edited
Loading

Landed in a3f47b1...d85a933 🎉

@BridgeAR BridgeAR closed this May 23, 2020
BridgeAR pushed a commit that referenced this pull request May 23, 2020
@bnoordhuis @BridgeAR
src: remove BeforeExit callback list
8ddb6aa 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this pull request May 23, 2020
@bnoordhuis @BridgeAR
wasi,worker: handle termination exception
d85a933 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: #33377

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@bnoordhuis @codebytere
src: remove BeforeExit callback list
f669ee2 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@bnoordhuis @codebytere
wasi,worker: handle termination exception
b8a696b 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: #33377

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@bnoordhuis @codebytere
src: remove BeforeExit callback list
d1e1dbf 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@bnoordhuis @codebytere
wasi,worker: handle termination exception
d153832 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: #33377

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@bnoordhuis @codebytere
src: remove BeforeExit callback list
e0680cc 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@bnoordhuis @codebytere
wasi,worker: handle termination exception
3d7d6d4 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: #33377

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
@bnoordhuis @codebytere
src: remove BeforeExit callback list
77b6298 
It obscures the fact that there is only a single BeforeExit action.
Just call that statically from `EmitBeforeExit()`.

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 8, 2020
@bnoordhuis @codebytere
wasi,worker: handle termination exception
7df79f4 
Be careful when emitting the 'beforeExit' event. It's not allowed to
call into the runtime when a termination exception is pending.

Fixes: #33377

PR-URL: #33386
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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++. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WASI: Crash when terminating a worker thread
8 participants
@bnoordhuis @cjihrig @nodejs-github-bot @BridgeAR @jasnell @addaleax @benjamingr @richardlau