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: discard tasks posted to platform TaskRunner during shutdown #31853

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Following the discussion in https://chromium-review.googlesource.com/c/v8/v8/+/2061548:

src: discard tasks posted to platform TaskRunner during shutdown

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

Revert "src: keep main-thread Isolate attached to platform during Dispose"

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

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

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
…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
@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 18, 2020
@addaleax addaleax added v8 platform Issues and PRs related to Node's v8::Platform implementation. and removed process Issues and PRs related to the process subsystem. labels Feb 18, 2020
@addaleax
Copy link
Member Author

After thinking about it, I think this also means that we can and should let go of the NodePlatform::DrainTasks() calls before disposing an Isolate. I’ll pursue that separately, though … no need to mix that in with a bug fix :)

src/node_platform.cc Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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>
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 <joyeec9h3@gmail.com>
@addaleax
Copy link
Member Author

Landed in d7fe554...61f28fb

@addaleax addaleax closed this Mar 11, 2020
@addaleax addaleax deleted the fix-31752 branch March 11, 2020 15:39
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>
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 <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
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 codebytere mentioned this pull request Mar 24, 2020
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>
@targos
Copy link
Member

targos commented Apr 22, 2020

@addaleax can this now land on v12.x or does it depend on something else?

@addaleax
Copy link
Member Author

@targos Yes, I think this can go ahead.

@targos targos removed lts-watch-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020
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 <joyeec9h3@gmail.com>
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 <joyeec9h3@gmail.com>
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++. 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.

Assertion failed in 12.16.0
5 participants