src: unregister Isolate with platform before disposing - #30909
Closed
addaleax wants to merge 1 commit into
Closed
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
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
codebytere
approved these changes
Dec 12, 2019
codebytere
left a comment
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
good catch - ty for the explainer on why it is in fact necessary!
Sorry, something went wrong.
richardlau
approved these changes
Dec 12, 2019
gengjiawen
approved these changes
Dec 12, 2019
devsnek
approved these changes
Dec 12, 2019
bnoordhuis
approved these changes
Dec 12, 2019
cjihrig
approved these changes
Dec 12, 2019
devnexen
approved these changes
Dec 12, 2019
Trott
approved these changes
Dec 13, 2019
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
Copy link
Copy Markdown
Collaborator
Sorry, something went wrong.
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Merged
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <codebytere@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Merged
Copy link
Copy Markdown
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 |
Sorry, something went wrong.
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
2 tasks
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 <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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 <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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 <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Copy link
Copy Markdown
Member
|
IIUC, this is no longer removed from v12.x |
Sorry, something went wrong.
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
3 tasks
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 <joyeec9h3@gmail.com>
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 <joyeec9h3@gmail.com>
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 <joyeec9h3@gmail.com>
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 <joyeec9h3@gmail.com>
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 <joyeec9h3@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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