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.