src: prepare platform for upstream V8 changes by addaleax · Pull Request #15428 · nodejs/node · GitHub

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises.

To address that:

Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten.
Move the task queue drain call before EmitBeforeExit() and only run EmitBeforeExit() if there is no new event loop work.
Account for possible new foreground tasks scheduled by background tasks in DrainBackgroundTasks().

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
- [ ] tests and/or benchmarks are included (no, because right now everything still works without this – in the future, some things like WASM would fail otherwise)
commit message follows commit guidelines

Affected core subsystem(s)

src/node_platform

/cc @nodejs/v8 @matthewloring @gahaas @natorion

Oooo big +1.... still have to review but this is definitely good to see.

Perhaps make this a flag rather than a bool so it's more self-describing in line... e.g.

InternalCallbackScope cb_scope(&env, Local<Object>(), { 0, 0 }, kAllowEmpty);

Or some such...

What exactly does this scope provide? Should it also be added to the places in node_platform.cc that invoke foreground tasks?

What exactly does this scope provide?

Calling async_hooks hooks + running the microtask queue and/or nextTicks

Should it also be added to the places in node_platform.cc that invoke foreground tasks?

Hm – I think so, yes. Thanks for pointing that out.

FWIW, it would be exceedingly helpful if we had actual documentation for how and when things like InternalCallbackScope are to be used.

@jasnell I agree

@matthewloring I will add this to the other places but that gets a bit tricky, because the platform only sees Isolates but looking up the proper Environment for a task is not trivial.

Nice!

@jasnell @matthewloring Done, updated!

@fhinkel This has changed a bit, do you want to take another look?

What we probably ought to get implemented on top of this change is actual async_hooks trackability for platform tasks, but I think that’s out of the scope of this PR + might require a bit more deliberation and/or chatting with the V8 team.

CI: https://ci.nodejs.org/job/node-test-commit/12441/

Unnecessary scoping?

Thanks, yes, it’s an editing leftover.

This method is intrinsically race-y. Maybe change FlushForegroundTasksInternal() to return true when foreground tasks were dispatched and update the loop in DrainBackgroundTasks() to something like this?

while (FlushForegroundTasksInternal())
  background_tasks_.BlockingDrain();

@bnoordhuis Fwiw, I don’t think the current code is actually race-y since, at least currently, only background tasks would enqueue new foreground tasks from another thread.

I like your suggestion, and d5bd7c55c19d0d4aee5a9511b9e6df1374d0644d should implement it correctly, but I think it doesn’t actually change the semantics here.

If you mean it's not race-y now because it's always called from the right thread, yes, you're right. What I mean is that the method itself is race-y by design.

Can you add some code comments here that describe briefly the difference between these flags

@jasnell done + rebased

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`.

Can you capitalize and punctuate the comment?

Sure, done!

Fresh CI: https://ci.nodejs.org/job/node-test-commit/12480/

Landed in f27b5e4

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: #15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: nodejs/node#15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: nodejs#15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

@MylesBorins I think we should backport this, yes. Like @TimothyGu said, it should come with #15639 – I’ll open a backport PR once both have landed.

Same for #15691 as well – it still makes sense to me to backport all of these 3 together.

nodejs#15428 was supposed to account for upcoming changes in V8 upstream, but while addressing review comments a bug was introduced; `DrainBackgroundTasks()` should always at least perform one blocking drain on the background task queue. PR-URL: nodejs#15639 Refs: nodejs#15428 Refs: f27b5e4 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). Fixes: nodejs#15672 Ref: nodejs#15428 Ref: f27b5e4

nodejs/node#15428 was supposed to account for upcoming changes in V8 upstream, but while addressing review comments a bug was introduced; `DrainBackgroundTasks()` should always at least perform one blocking drain on the background task queue. PR-URL: nodejs/node#15639 Refs: nodejs/node#15428 Refs: f27b5e4bdaafc73a830a0451ee3c641b8bcd08fe Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). PR-URL: #15691 Fixes: #15672 Ref: #15428 Ref: f27b5e4 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). PR-URL: nodejs/node#15691 Fixes: nodejs/node#15672 Ref: nodejs/node#15428 Ref: f27b5e4bdaafc73a830a0451ee3c641b8bcd08fe Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: nodejs#15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

nodejs#15428 was supposed to account for upcoming changes in V8 upstream, but while addressing review comments a bug was introduced; `DrainBackgroundTasks()` should always at least perform one blocking drain on the background task queue. PR-URL: nodejs#15639 Refs: nodejs#15428 Refs: f27b5e4 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). PR-URL: nodejs#15691 Fixes: nodejs#15672 Ref: nodejs#15428 Ref: f27b5e4 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: #15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

#15428 was supposed to account for upcoming changes in V8 upstream, but while addressing review comments a bug was introduced; `DrainBackgroundTasks()` should always at least perform one blocking drain on the background task queue. PR-URL: #15639 Refs: #15428 Refs: f27b5e4 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). PR-URL: #15691 Fixes: #15672 Ref: #15428 Ref: f27b5e4 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

V8 platform tasks may schedule other tasks (both background and foreground), and may perform asynchronous operations like resolving Promises. To address that: - Run the task queue drain call inside a callback scope. This makes sure asynchronous operations inside it, like resolving promises, lead to the microtask queue and any subsequent operations not being silently forgotten. - Move the task queue drain call before `EmitBeforeExit()` and only run `EmitBeforeExit()` if there is no new event loop work. - Account for possible new foreground tasks scheduled by background tasks in `DrainBackgroundTasks()`. PR-URL: #15428 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Matthew Loring <mattloring@google.com>

#15428 was supposed to account for upcoming changes in V8 upstream, but while addressing review comments a bug was introduced; `DrainBackgroundTasks()` should always at least perform one blocking drain on the background task queue. PR-URL: #15639 Refs: #15428 Refs: f27b5e4 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). PR-URL: #15691 Fixes: #15672 Ref: #15428 Ref: f27b5e4 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This does not land cleanly in v6.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 15, 2017

addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Sep 15, 2017

jasnell reviewed Sep 15, 2017

View reviewed changes

matthewloring reviewed Sep 15, 2017

View reviewed changes

fhinkel approved these changes Sep 16, 2017

View reviewed changes

addaleax force-pushed the fix-node-platform branch from a7e7fff to f078c66 Compare September 19, 2017 13:49

jasnell approved these changes Sep 19, 2017

View reviewed changes

bnoordhuis reviewed Sep 19, 2017

View reviewed changes

addaleax force-pushed the fix-node-platform branch 3 times, most recently from a60d5ee to f311ed9 Compare September 19, 2017 18:16

addaleax mentioned this pull request Sep 20, 2017

fs: initial experimental promisified API #15485

Closed

4 tasks

jasnell reviewed Sep 20, 2017

View reviewed changes

addaleax added 3 commits September 20, 2017 16:23

[squash] implement bnoordhuis suggestion

75bb3c4

[squash] add requested comment

83c3e74

addaleax force-pushed the fix-node-platform branch from f311ed9 to 83c3e74 Compare September 20, 2017 14:24

bnoordhuis approved these changes Sep 20, 2017

View reviewed changes

[squash] capitalize + punctuate

f7802f5

matthewloring approved these changes Sep 20, 2017

View reviewed changes

addaleax closed this Sep 21, 2017

addaleax deleted the fix-node-platform branch September 21, 2017 00:04

addaleax mentioned this pull request Sep 28, 2017

Node.js Crash #15652

Closed

addaleax mentioned this pull request Sep 29, 2017

src: use more appropriate context-entered check #15691

Closed

3 tasks

addaleax added a commit to addaleax/node that referenced this pull request Oct 2, 2017

src: use more appropriate context-entered check

c1c5862

Make the context check in `MakeCallback` match what the comment says (and what actually makes sense). Fixes: nodejs#15672 Ref: nodejs#15428 Ref: f27b5e4

addaleax mentioned this pull request Oct 8, 2017

[v8.x] src: prepare platform for upstream V8 changes #16091

Closed

targos added backported-to-v8.x and removed backport-requested-v8.x labels Oct 18, 2017

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

addaleax added the v8 platform Issues and PRs related to Node's v8::Platform implementation. label Feb 18, 2020

cspotcode mentioned this pull request Jan 17, 2022

tiered WebAssembly compilation broken #36616

Closed

                 void DrainBackgroundTasks();
-                void FlushForegroundTasksInternal();
+                // returns true iff work was dispatched or executed

src: prepare platform for upstream V8 changes by addaleax · Pull Request #15428 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: prepare platform for upstream V8 changes - #15428

src: prepare platform for upstream V8 changes#15428addaleax wants to merge 4 commits intonodejs:masternodejs/node:masterfrom addaleax:fix-node-platformaddaleax/node:fix-node-platformCopy head branch name to clipboard

Conversation

addaleax commented Sep 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 20, 2017

Uh oh!

addaleax commented Sep 21, 2017

Uh oh!

addaleax commented Sep 28, 2017

Uh oh!

addaleax commented Sep 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: prepare platform for upstream V8 changes#15428
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:fix-node-platform