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

src: check microtasks before running them #29434

Closed
wants to merge 1 commit into from
Closed

src: check microtasks before running them #29434

wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 4, 2019 •
edited
Loading

Refs electron/electron#20013.

Previously, Node.js unilaterally ran microtasks on next tick; since Electron also handles microtask resolution this created an issue whereby the microtask queue would attempt to be pumped while previous tasks were still running.

To address this issue, i've changed the RunMicrotasks call to a PerformCheckpoint call, which adds checks to only run microtasks if none are in progress, and otherwise proceeds normally.

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 4, 2019
@codebytere codebytere mentioned this pull request Sep 4, 2019
Closed
@nodejs-github-bot
Copy link
Collaborator

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

bnoordhuis
bnoordhuis approved these changes Sep 4, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

MicrotasksScope::PerformCheckpoint() is basically Isolate::RunMicrotasks() with a nesting guard, right? LGTM if that's the case.

@nodejs-github-bot
Copy link
Collaborator

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

@codebytere
Copy link
Member Author

codebytere commented Sep 4, 2019 •
edited
Loading

@bnoordhuis yep! It just runs conditionally on:

!IsRunningMicrotasks() && !GetMicrotasksScopeDepth() && !HasMicrotasksSuppressions()

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 4, 2019
@Trott
Copy link
Member

Trott commented Sep 4, 2019

Can't think of a simple/reliable way to test this, so LGTM without one, but commenting in case someone else has an idea.

Trott pushed a commit that referenced this pull request Sep 6, 2019
@codebytere @Trott
src: check microtasks before running them
63b056d 
PR-URL: #29434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@Trott
Copy link
Member

Trott commented Sep 6, 2019

Landed in 63b056d

@Trott Trott closed this Sep 6, 2019
@codebytere codebytere deleted the check-microtask-before-run branch September 6, 2019 16:05
nornagon added a commit to nornagon/node-1 that referenced this pull request Sep 16, 2019
@nornagon
src: runMicrotasks should check before running too
8cda9c2 
This is a continuation of nodejs#29434, rewriting the last remaining call to RunMicrotasks.
@nornagon nornagon mentioned this pull request Sep 16, 2019
Closed
4 tasks
targos pushed a commit that referenced this pull request Sep 20, 2019
@codebytere @targos
src: check microtasks before running them
5c3d484 
PR-URL: #29434
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@nornagon nornagon mentioned this pull request Sep 20, 2019
Merged
6 tasks
Trott pushed a commit that referenced this pull request Sep 23, 2019
@nornagon @Trott
src: perform check before running in runMicrotasks()
4ef3ccb 
This is a continuation of #29434,
rewriting the last remaining call to RunMicrotasks.

PR-URL: #29581
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 23, 2019
@nornagon @targos
src: perform check before running in runMicrotasks()
8b55ccb 
This is a continuation of #29434,
rewriting the last remaining call to RunMicrotasks.

PR-URL: #29581
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@nornagon @BridgeAR
src: perform check before running in runMicrotasks()
3878e1e 
This is a continuation of #29434,
rewriting the last remaining call to RunMicrotasks.

PR-URL: #29581
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
codebytere added a commit to electron/electron that referenced this pull request Oct 15, 2019
@codebytere
update fixme_remove_async_id_assertion_check.patch
9f50a03 
nodejs/node#29434
codebytere added a commit to electron/electron that referenced this pull request Oct 15, 2019
@codebytere
update fixme_remove_async_id_assertion_check.patch
ff4e402 
nodejs/node#29434
codebytere added a commit to electron/electron that referenced this pull request Oct 17, 2019
@codebytere
update fixme_remove_async_id_assertion_check.patch
e22c90c 
nodejs/node#29434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@devnexen devnexen devnexen approved these changes

@JungMinu JungMinu JungMinu approved these changes

@devsnek devsnek devsnek approved these changes

@BridgeAR BridgeAR BridgeAR 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++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.