Node: Microtask queue is not flushed when promises are fulfilled in C++

[vkurchatkin]

Reproduction:

#include <node.h>
#include <uv.h>
#include <unistd.h>

static v8::Persistent<v8::Promise::Resolver> persistent;

void run(uv_work_t* req) {
  sleep(1);
}

void callback(uv_work_t* req, int i) {
  v8::Isolate* isolate = v8::Isolate::GetCurrent();
  v8::HandleScope scope(isolate);

  v8::Local<v8::Promise::Resolver> local = v8::Local<v8::Promise::Resolver>::New(isolate, persistent);
  local->Resolve(v8::Undefined(isolate));
}

void test(const v8::FunctionCallbackInfo<v8::Value>& args) {
  v8::Isolate* isolate = args.GetIsolate();

  if (persistent.IsEmpty()) {
    persistent.Reset(isolate, v8::Promise::Resolver::New(isolate));

    uv_work_t * req = (uv_work_t*) malloc(sizeof(uv_work_t));

    uv_queue_work(uv_default_loop(), req, run, callback);
  }

  v8::Local<v8::Promise::Resolver> local = v8::Local<v8::Promise::Resolver>::New(isolate, persistent);

  args.GetReturnValue().Set(local->GetPromise());
}

void init(v8::Local<v8::Object> exports) {
  NODE_SET_METHOD(exports, "test", test);
}

NODE_MODULE(addon, init)

var r = require('./build/Release/addon.node');

r.test().then(function() {
  console.log('done');
});


setTimeout(() => {}, 5000);

done should be printed after 1 seconds, but is printed after 5 seconds instead.

[vkurchatkin]

It seems that calling isolate->SetAutorunMicrotasks(true); doesn't help with this problem

[vkurchatkin]

One possible solution: add CallbackScope class that extends functionality of MakeCallback:

v8::Isolate* isolate = v8::Isolate::GetCurrent();
v8::HandleScope scope(isolate);
node::CallbackScope callbackScope(isolate);

 v8::Local<v8::Promise::Resolver> local = v8::Local<v8::Promise::Resolver>::New(isolate, persistent);
 local->Resolve(v8::Undefined(isolate));

// resolve more promises, call functions, whatever

When CallbackScope is exited, env->KickNextTick is called automatically.

[IdeaHunter]

Another possible solution suggested by @ranisalt:

local->Resolve(v8::Undefined(isolate));
//give it a kick
Isolate::GetCurrent()->RunMicrotasks();

But it still doesn't clear - should it be our responsibility to add RunMicrotasks or CallbackScope or v8 should handle it itself...

[vkurchatkin]

Just running microtasks is not enough, you need to run next tick queue as well.

[ranisalt]

@IdeaHunter I couldn't get it to work on a more complex example, I guess promise resolving/rejecting is not considered a microtask.

How's a simple way to get the current environment? I searched through code but couldn't find it. In fact I'm only getting a forward declaration of Environment in my source and thus can not use the class at all.

[benjamingr]

@vkurchatkin would you mind clarifying how things run in particular and why running the microtask queue is not enough and what "kicks" the continuation?

When I checked comparable code in blink I didn't find anything similar required (again, I'm far far far from an experts in these parts).

[vkurchatkin]

@benjamingr the proper way to call JS from C++ is to use MakeCallback. It does all necessary things before and after calling the function, including running next tick and microtask queues.

[benjamingr]

@vkurchatkin and just to make sure I understand:

We never actually have any callbacks here because everything is in C++ code, we're just returning a promise and resolve/reject that doesn't run nextTick and the microtask queue so nothing happens until that's triggered some other way.

I'm pondering about the possibility of asking V8 for something akin to bluebird's setScheduler (only in C++ land) that will allow consumers to choose how to schedule the callback and will solve this in addition to a lot of other issues we have with promises (like AsyncWrap). It would default to the same behavior it has now - what do you think?

[vkurchatkin]

@benjamingr correct. It also seems that it is implied by v8 authors that you should run microtasks manually when you work with promises from C++.

I'm pondering about the possibility of asking V8 for something akin to bluebird's setScheduler

@chrisdickinson is working on something like this. It won't solve this problem, though. Users still need to flush scheduled callbacks explicitly at some point.

[benjamingr]

Users still need to flush scheduled callbacks explicitly at some point.

Basically what we'd like (optimally) is for resolve and reject to schedule a microtask flush if one is not already present. I get not doing so automatically though and this is a change we can't really ask for anyway.

If we can't get there I agree it needs to be documented very clearly at least.

[vkurchatkin]

I get not doing so automatically though and this is a change we can't really ask for anyway.

I'm not 100% sure, I expected that V8 does that, but seems like it does not. Running microtasks each would be suboptimal and can be confusing, because promise handlers are expected to be executed asynchronously and they will be synchronous in this case.

[benjamingr]

I meant as with a trampoline - as in "set a boolean flag to true and later if it's true run the handlers" and keep running them until none are left.

There is no reason to assume they'd do it like that since the API in C++ is lower level. I just assumed it since that's what promise libraries do.

[vkurchatkin]

s in "set a boolean flag to true and later if it's true run the handlers" and keep running them until none are left.

@benjamingr the problem is that there is no definite "later" in this case

[vkurchatkin]

Proof of concept:

diff --git a/src/node.cc b/src/node.cc
index c0e8dbc..c713509 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1287,6 +1287,14 @@ Local<Value> MakeCallback(Isolate* isolate,
         MakeCallback(env, recv.As<Value>(), callback, argc, argv)));
 }

+CallbackScope::CallbackScope(Isolate* isolate): isolate_(isolate) {
+}
+
+CallbackScope::~CallbackScope() {
+  Environment* env = Environment::GetCurrent(isolate_);
+  Environment::AsyncCallbackScope callback_scope(env);
+  env->KickNextTick(&callback_scope);
+}

 enum encoding ParseEncoding(const char* encoding,
                             enum encoding default_encoding) {
diff --git a/src/node.h b/src/node.h
index fcbb450..2ab3768 100644
--- a/src/node.h
+++ b/src/node.h
@@ -41,6 +41,7 @@

 #include "v8.h"  // NOLINT(build/include_order)
 #include "node_version.h"  // NODE_MODULE_VERSION
+#include "util.h"

 #define NODE_MAKE_VERSION(major, minor, patch)                                \
   ((major) * 0x1000 + (minor) * 0x100 + (patch))
@@ -150,6 +151,17 @@ NODE_EXTERN v8::Local<v8::Value> MakeCallback(
     int argc,
     v8::Local<v8::Value>* argv);

+class NODE_EXTERN CallbackScope {
+   public:
+    explicit CallbackScope(v8::Isolate* isolate);
+    ~CallbackScope();
+
+   private:
+    v8::Isolate* isolate_;
+
+    DISALLOW_COPY_AND_ASSIGN(CallbackScope);
+};
+
 }  // namespace node

 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

Then in addon:

void callback(uv_work_t* req, int i) {
  v8::Isolate* isolate = v8::Isolate::GetCurrent();
  v8::HandleScope scope(isolate);
  node::CallbackScope callback_scope(isolate);

  v8::Local<v8::Promise::Resolver> local = v8::Local<v8::Promise::Resolver>::New(isolate, persistent);
  local->Resolve(v8::Undefined(isolate));
}

We can probably combine HandleScope and CallbackScope for convenience. The best thing about this approach is that it doesn't matter if we use callbacks or promises or both.

[benjamingr]

I like the RAII here, the only issue is that it's always coupled with Resolve anyway - that is, you want that CallbackScope there if and only if you call Resolve and Reject in that scope - if the call to resolution is conditional, or happens "later" (as in a C++ callback) then the RAII doesn't really work.

It might as well be a

ResolvePromise(local, v8::Undefined(isolate))

and save me the extra line - no?

[vkurchatkin]

ResolvePromise(local, v8::Undefined(isolate))

This works only for single promise, CallbackScope works for multiple calls to js, any calls.

you want that CallbackScope there if and only if you call Resolve and Reject in that scope

You can set up scope conditionally, but that seems to be rarely useful.

[benjamingr]

@vkurchatkin any update on this?

[Trott]

Should this remain open?

[benjamingr]

@Trott I believe so - waiting for an update from @vkurchatkin

[addaleax]

I think this has not been fixed, and my guess would be that it’ll never really be fixed. I’m not sure whether it was around when this issue was opened, but maybe v8::Isolate::RunMicrotasks() is the best answer we can get here?

[vkurchatkin]

v8::Isolate::RunMicrotasks() is the best answer we can get here?

We should expose a proper way to flush microtask and nextTick queues for old API as well as N-API.

[addaleax]

I think https://github.com/nodejs/node/pull/14697 would be a valid fix for this, added a regression test for it in a4505910d29597ff8fd9f95d4effcbf3196f63ca