node: support v8 microtask queue

[vkurchatkin]

Since v8 introduced Promise support it became possible to schedule tick callback which will be executed only after next javascript invocation:

Promise.resolve().then(function () {
  process.nextTick(function () {
    console.log('tick');
  });
});

setTimeout(function () {
  console.log('timeout');
}, 2000);

It may look like this issue affects only first tick, but it's not:

setTimeout(function () {
  process.nextTick(function () {
    Promise.resolve().then(function () {
      process.nextTick(function () {
        console.log('tick');
      });
    });

    setTimeout(function () {
      console.log('timeout');
    }, 2000);
  });
}, 0);

This happens because v8 uses microtask queue to run promise callbacks (and also Object.observe callbacks), which is essentially the same as nextTick queue. Together they don't play really nice: nextTick callback may schedule a microtask and vice versa. If queues are flushed in any particular order some callbacks won't be called. To handle this problem we need to flush queues in a loop until both are empty.

This PR implements the following algorithm:

1. Execute all callbacks, that are CURRENTLY present in nextTick queue (not the ones added recursively)
2. Flush microtask queue
3. Check if there are more callbacks in nextTick queue, if so, go to 1.

This attempts to make two microtasks queues behave as one. Unfortunately, microtask queue has higher priority and can potentially prevent nextTick callbacks added before from running:

setTimeout(function () {
  Promise.resolve().then(function () {
    process.nextTick(function () {
      console.log('never printed');
    });

    function block () {
      Promise.resolve().then(block);
    }

    block();
  });
}, 0)

It looks like there is nothing we can do about it without direct control over microtask queue. For same reason there is no way to make microtask queue work with domains.

fixes #7714

/cc @tjfontaine @trevnorris @indutny

[bnoordhuis]

Your approach looks okay to me but I'm curious what made you decide to go with this instead of reimplementing process.nextTick() in terms of V8::EnqueueMicrotask()?

It is because V8::RunMicrotasks() breaks when a callback throws an exception? That has been fixed in upstream V8 although in a way that is slightly incompatible with process.nextTick(): V8::RunMicrotasks() drops pending microtasks whereas process.nextTick() continues on the next tick (which isn't particularly sound IMO, but that aside.)

[vkurchatkin]

@bnoordhuis yeah, mostly. I'm afraid of other possible subtle incompatibilities between EnqueueMicrotask and process.nextTick too. Also this would break domain support, async listeners, etc. I think it's too risky to delegate nextTick to unstable microtask implementation, at least for now.

[vkurchatkin]

@tjfontaine @trevnorris @indutny ping

[trevnorris]

You can drop _asyncQueue. It'll be removed soon anyway.

[trevnorris]

Well, did a quick performance test and the times are comparable between passing a function to EnqueueMicrotask() and nextTick(). If the implementation could be done safely then I'd be fine switching to using EnqueueMicrotask().

[shigeki]

#8376 is an another approach to solve #7714 and it also fixes callback order to always execute nextTick at the end of JS excursion.

[vkurchatkin]

@trevnorris switching to EnqueueMicrotask is problematic (see @bnoordhuis comment):

• this requires updating v8; now if a microtask throws and RunMicrotasks runs inside try-catch, v8 crashes;
• async listeneres and domains will not work:
• RunMicrotasks drops queue on exception.

@shigeki your PR doesn't fix this issue, only one particular case.

[trevnorris]

@vkurchatkin Yeah, I realize that it's currently not possible. Hence my "If the implementation could be done safely." :-)

[trevnorris]

@vkurchatkin Please apply the following diff. I like to prevent pollution of the process object. Event if it's momentary.

diff --git a/src/node.cc b/src/node.cc
index c1f6d20..2223c36 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -978,6 +978,7 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {

   assert(args[0]->IsObject());
   assert(args[1]->IsFunction());
+  assert(args[2]->IsObject());

   // Values use to cross communicate with processNextTick.
   Local<Object> tick_info_obj = args[0].As<Object>();
@@ -988,6 +989,8 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {

   env->set_tick_callback_function(args[1].As<Function>());

+  NODE_SET_METHOD(args[2].As<Object>(), "runMicrotasks", RunMicrotasks);
+
   // Do a little housekeeping.
   env->process_object()->Delete(
       FIXED_ONE_BYTE_STRING(args.GetIsolate(), "_setupNextTick"));
@@ -2804,8 +2807,6 @@ void SetupProcessObject(Environment* env,
   NODE_SET_METHOD(process, "_setupNextTick", SetupNextTick);
   NODE_SET_METHOD(process, "_setupDomainUse", SetupDomainUse);

-  NODE_SET_METHOD(process, "_runMicrotasks", RunMicrotasks);
-
   // pre-set _events object for faster emit checks
   process->Set(env->events_string(), Object::New(env->isolate()));
 }
diff --git a/src/node.js b/src/node.js
index 064a005..e7b45ad 100644
--- a/src/node.js
+++ b/src/node.js
@@ -294,10 +294,10 @@
     var _runAsyncQueue = tracing._runAsyncQueue;
     var _loadAsyncQueue = tracing._loadAsyncQueue;
     var _unloadAsyncQueue = tracing._unloadAsyncQueue;
-    var _runMicrotasks = process._runMicrotasks;
     var microtasksScheduled = false;

-    delete process._runMicrotasks;
+    // Used to run V8's micro task queue.
+    var _runMicrotasks = {};

     // This tickInfo thing is used so that the C++ code in src/node.cc
     // can have easy accesss to our nextTick state, and avoid unnecessary
@@ -316,7 +316,9 @@
     process._tickCallback = _tickCallback;
     process._tickDomainCallback = _tickDomainCallback;

-    process._setupNextTick(tickInfo, _tickCallback);
+    process._setupNextTick(tickInfo, _tickCallback, _runMicrotasks);
+
+    _runMicrotasks = _runMicrotasks.runMicrotasks;

     function tickDone() {
       if (tickInfo[kLength] !== 0) {

Then ping me back. Should be ready to roll.

[vkurchatkin]

@trevnorris all set

[trevnorris]

Thanks. Merged into 30bd7b6.

[domenic]

\o/ very excited. A new 0.11 release would be much appreciated if it's not too much trouble. (Which it might be.)

[Fishrock123]

🍻 Been waiting on this haha.