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

src: unregister Isolate with platform before disposing #30909

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

/cc @codebytere

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: nodejs#30846
Refs: nodejs#30181
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 12, 2019
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

good catch - ty for the explainer on why it is in fact necessary!

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Dec 14, 2019

Landed in 25447d8

@Trott Trott closed this Dec 14, 2019
Trott pushed a commit that referenced this pull request Dec 14, 2019
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d.

Fixes: #30846
Refs: #30181

PR-URL: #30909
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
@MylesBorins
Copy link
Contributor

This caused some regression on v12.x so I'm going to revert it in 12.16.1

Adding backport label so we can still get it landed

@addaleax addaleax deleted the revert-30181 branch February 14, 2020 15:11
addaleax added a commit to addaleax/node that referenced this pull request Feb 14, 2020
This works around a situation in which the V8 WASM code calls
into the platform while the Isolate is being disposed.

This goes against the V8 API constract for `v8::Platform`.
In lieu of a proper fix, it should be okay to keep the Isolate
registered; the race condition fixed by 25447d8 cannot
occur for the `NodeMainInstance`’s Isolate, as it is the last
one to exit in any given Node.js process.

This partially reverts 25447d8.

Refs: nodejs#30909
Refs: nodejs#31752
addaleax added a commit that referenced this pull request Feb 14, 2020
This works around a situation in which the V8 WASM code calls
into the platform while the Isolate is being disposed.

This goes against the V8 API constract for `v8::Platform`.
In lieu of a proper fix, it should be okay to keep the Isolate
registered; the race condition fixed by 25447d8 cannot
occur for the `NodeMainInstance`’s Isolate, as it is the last
one to exit in any given Node.js process.

This partially reverts 25447d8.

Refs: #30909
Refs: #31752

PR-URL: #31795
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Feb 14, 2020
This works around a situation in which the V8 WASM code calls
into the platform while the Isolate is being disposed.

This goes against the V8 API constract for `v8::Platform`.
In lieu of a proper fix, it should be okay to keep the Isolate
registered; the race condition fixed by 25447d8 cannot
occur for the `NodeMainInstance`’s Isolate, as it is the last
one to exit in any given Node.js process.

This partially reverts 25447d8.

Refs: #30909
Refs: #31752

PR-URL: #31795
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
This works around a situation in which the V8 WASM code calls
into the platform while the Isolate is being disposed.

This goes against the V8 API constract for `v8::Platform`.
In lieu of a proper fix, it should be okay to keep the Isolate
registered; the race condition fixed by 25447d8 cannot
occur for the `NodeMainInstance`’s Isolate, as it is the last
one to exit in any given Node.js process.

This partially reverts 25447d8.

Refs: #30909
Refs: #31752

PR-URL: #31795
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member

targos commented Feb 18, 2020

IIUC, this is no longer removed from v12.x

addaleax added a commit to addaleax/node that referenced this pull request Feb 18, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: nodejs#31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: nodejs#31795
Refs: nodejs#30909
@addaleax addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020
addaleax added a commit that referenced this pull request Mar 11, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: #31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: #31795
Refs: #30909
PR-URL: #31853
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: #31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: #31795
Refs: #30909
PR-URL: #31853
Reviewed-By: Joyee Cheung <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 21, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: #31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: #31795
Refs: #30909
PR-URL: #31853
Reviewed-By: Joyee Cheung <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 23, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: #31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: #31795
Refs: #30909
PR-URL: #31853
Reviewed-By: Joyee Cheung <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Discard tasks silently that are posted when the Isolate is being
disposed.

It is not possible to avoid a race condition window between
unregistering the Isolate with the platform and disposing it
in which background tasks and the Isolate deinit steps themselves
may lead to new tasks being posted. The only sensible action
in that case is discarding the tasks.

Fixes: #31752
Fixes: https://bugs.chromium.org/p/v8/issues/detail?id=10104
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/2061548
Refs: #31795
Refs: #30909
PR-URL: #31853
Reviewed-By: Joyee Cheung <[email protected]>
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++. lib / src Issues and PRs related to general changes in the lib or src directory. v8 platform Issues and PRs related to Node's v8::Platform implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test parallel/test-worker-message-port-transfer-terminate