src: prepare platform for upstream V8 changes #15428

addaleax · 2017-09-15T13:44:00Z

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

jasnell · 2017-09-15T16:00:11Z

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

jasnell · 2017-09-15T16:02:12Z

src/node.cc

@@ -1352,7 +1352,8 @@ class InternalCallbackScope {
 public:
  InternalCallbackScope(Environment* env,
                        Local<Object> object,
-                        const async_context& asyncContext);
+                        const async_context& asyncContext,
+                        bool allow_empty_object = false);


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...

matthewloring · 2017-09-15T19:55:27Z

src/node.cc

@@ -4747,9 +4754,17 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
    do {
      uv_run(env.event_loop(), UV_RUN_DEFAULT);

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


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.

fhinkel

Nice!

addaleax · 2017-09-19T13:50:20Z

@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/

bnoordhuis · 2017-09-19T14:51:30Z

src/node.cc

@@ -4747,9 +4729,16 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
    do {
      uv_run(env.event_loop(), UV_RUN_DEFAULT);

+      {
+        v8_platform.DrainVMTasks();
+      }


Unnecessary scoping?

Thanks, yes, it’s an editing leftover.

bnoordhuis · 2017-09-19T15:03:21Z

src/node_platform.cc

+bool TaskQueue<T>::HasPending() {
+  Mutex::ScopedLock scoped_lock(lock_);
+  return !task_queue_.empty();
+}


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.

jasnell · 2017-09-20T14:20:58Z

src/node_internals.h

@@ -294,8 +294,35 @@ v8::MaybeLocal<v8::Value> InternalMakeCallback(
    v8::Local<v8::Value> argv[],
    async_context asyncContext);

+class InternalCallbackScope {
+ public:
+  enum ResourceExpectation { kRequireResource, kAllowEmptyResource };


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()`.

bnoordhuis · 2017-09-20T17:05:12Z

src/node_platform.h

@@ -39,7 +39,8 @@ class NodePlatform : public v8::Platform {
  virtual ~NodePlatform() {}

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


Can you capitalize and punctuate the comment?

Sure, done!

addaleax · 2017-09-20T17:28:30Z

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

addaleax · 2017-09-21T00:04:06Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

addaleax · 2017-09-28T17:03:31Z

@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.

addaleax · 2017-09-29T21:41:27Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

#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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matthew Loring <[email protected]>

#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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

MylesBorins · 2017-11-16T19:03:36Z

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: prepare platform for upstream V8 changes #15428

src: prepare platform for upstream V8 changes #15428

addaleax commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell Sep 15, 2017

matthewloring Sep 15, 2017

addaleax Sep 15, 2017

jasnell Sep 15, 2017

addaleax Sep 15, 2017

fhinkel left a comment

addaleax commented Sep 19, 2017

bnoordhuis Sep 19, 2017

addaleax Sep 19, 2017

bnoordhuis Sep 19, 2017

addaleax Sep 19, 2017

bnoordhuis Sep 20, 2017

jasnell Sep 20, 2017

addaleax Sep 20, 2017

bnoordhuis Sep 20, 2017

addaleax Sep 20, 2017

addaleax commented Sep 20, 2017

addaleax commented Sep 21, 2017

addaleax commented Sep 28, 2017

addaleax commented Sep 29, 2017

MylesBorins commented Nov 16, 2017

src: prepare platform for upstream V8 changes #15428

src: prepare platform for upstream V8 changes #15428

Conversation

addaleax commented Sep 15, 2017

Checklist

Affected core subsystem(s)

jasnell commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel left a comment

Choose a reason for hiding this comment

addaleax commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Sep 20, 2017

addaleax commented Sep 21, 2017

addaleax commented Sep 28, 2017

addaleax commented Sep 29, 2017

MylesBorins commented Nov 16, 2017