Skip to content

src: check microtasks before running them#29434

Closed
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:check-microtask-before-run
Closed

src: check microtasks before running them#29434
codebytere wants to merge 1 commit intonodejs:masterfrom
codebytere:check-microtask-before-run

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

nodejs-github-bot commented Sep 4, 2019

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

nodejs-github-bot commented Sep 4, 2019

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

@JungMinu JungMinu JungMinu approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+2 more reviewers

@devnexen devnexen devnexen approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

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.