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: keep main-thread Isolate attached to platform during Dispose #31795

Conversation

addaleax
Copy link
Member

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

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

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
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Feb 14, 2020
@addaleax addaleax added fast-track PRs that do not need to wait for 48 hours to land. lib / src Issues and PRs related to general changes in the lib or src directory. and removed process Issues and PRs related to the process subsystem. labels Feb 14, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

Please 👍 to fast-track this for inclusion in #31781 (v12.16.1).

@addaleax addaleax mentioned this pull request Feb 14, 2020
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM as a temporary fix but I definitely worry about breaking the contract here. Do we know what the WASM edge cases are and is there a better way of working around those?

@addaleax
Copy link
Member Author

@jasnell I don’t have a full understanding of the situation yet. It looks like the WASM engine in V8 has its own, separate GC feature, that can also be triggered during Isolate deinitialization (well, the stack trace in the issue basically confirms that).

My attempt to fix that would be basically to skip sending the task after Isolate::Deinit() has been entered. That does fix #31752, and I’m currently compiling V8 to run their test suite and see if that breaks anything. If that works, I’ll try to put together a regression test and open a V8 CL – we’ll see where it goes from there. (It’s generally my experience that proposing concrete changes get things done earlier than opening issues – both in V8 and Node.js 🙂)

but I definitely worry about breaking the contract here.

To be clear, my understanding is that V8 does that, not that we do that.

@addaleax
Copy link
Member Author

Landed in e460f8c

@addaleax addaleax closed this Feb 14, 2020
@addaleax addaleax deleted the node-main-instance-isolate-dispose-order branch February 14, 2020 16:15
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]>
@codebytere codebytere mentioned this pull request Feb 17, 2020
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 added a commit to addaleax/node that referenced this pull request Feb 18, 2020
…pose"

This reverts commit e460f8c.

It is no longer necessary after the previous commit, and restores
consistency of the call order between the main thread code,
the other call sites, and the documentation.

Refs: nodejs#31795
@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]>
addaleax added a commit that referenced this pull request Mar 11, 2020
…pose"

This reverts commit e460f8c.

It is no longer necessary after the previous commit, and restores
consistency of the call order between the main thread code,
the other call sites, and the documentation.

Refs: #31795
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]>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
…pose"

This reverts commit e460f8c.

It is no longer necessary after the previous commit, and restores
consistency of the call order between the main thread code,
the other call sites, and the documentation.

Refs: #31795
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]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
…pose"

This reverts commit e460f8c.

It is no longer necessary after the previous commit, and restores
consistency of the call order between the main thread code,
the other call sites, and the documentation.

Refs: nodejs#31795
PR-URL: nodejs#31853
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
…pose"

This reverts commit e460f8c.

It is no longer necessary after the previous commit, and restores
consistency of the call order between the main thread code,
the other call sites, and the documentation.

Refs: #31795
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++. fast-track PRs that do not need to wait for 48 hours to land. 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.

4 participants