AsyncWorker & Promise - is it possible?

Im trying to create a wrapper for native calls but have a problem with resolving promise inside HandleOKCallback . Here the code:

void MyWorker::Run(const Nan::FunctionCallbackInfo<Value>* info, Work work){
    auto resolver = Promise::Resolver::New(info->GetIsolate());
    auto worker = new MyWorker(work,resolver);
    worker->SaveToPersistent('key',resolver);
    AsyncQueueWorker(worker);
    info->GetReturnValue().Set(resolver->GetPromise());
}

void MyWorker::HandleOKCallback(){
    auto res = GetFromPersistent('key').As<v8::Promise::Resolver>();
    res->Resolve(Nan::Undefined());
}

The problem here is that the promise on js side getting resolved only when addon is unloaded...
What is the proper way to resolve promise inside HandleOKCallback?

I have never used promises from C++, so I do not know. You seem to be missing a HandleScope in each function, but I do not believe that to be the culprit. Perhaps the next tick is not advanced? You could try inserting a MakeCallback call to some js function and see if that changes anything.

@IdeaHunter

@IdeaHunter I tried updateing the AsyncWorker example from this repo with Promises (in the way you use them), and everything seems to work just fine, so I guess more context would be helpful.

Also, if this is a verbatim excerpt from your test case, you may want to keep in mind that 'key' in C++ is not a string, but rather a single char (e.g. 'key' == 'y' == 121), and using double quotes will more likely be what you expect. (Google for “multi-character character constant”, if you’re curious. :))

@kkoopa … and I thought I had finally figured out when HandleScopes are necessary. ;) For clarification: MyWorker::Run is very likely called from JS, directly or indirectly; Are you sure it needs its own HandleScope? If so, I think the phrasing in doc/scopes.md is somewhat misleading.

Since it takes a pointer as its first argument, it cannot be called directly, since all functions exposed to V8 take a const reference. When a function is called indirectly, which scope its handles will leak to depends on where it is called from. It probably does not matter in this case, but I could not come up with anything better to try. Note that HandleOKCallback probably is called from libuv and these things matter more then.

Okay, makes sense! :)

@addaleax

@addaleax Thank you for pointing at multi-char - i definitely missed that one.
About context: I do nothing more that calling MyWorker::Run directly from unit tests and unit tests getting timeout on resolving.

var test = require('bindings')('test');

describe('when running async worker', function () {
    it('resolve the promise without jasmine timeout', function (done) {
        console.log('~Test started~');
        test().then(function (argument) {
            console.log('~Resolved~');
            done();
        },done);
    });
});

When i ran the code above i can see in console that ~Resolved~ appear at very end of output that is definetly look strange for any scenario i can imagine:

[13:31:57] Using gulpfile ~/tx/wp/gulpfile.js
[13:31:57] Starting 'test'...
Running 1 spec.

when running async worker~Test started~

    resolve the promise without jasmine timeout: failed

Failures: 
1) when running async worker resolve the promise without jasmine timeout
1.1) Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

1 spec, 1 failure
Finished in 5 seconds
[13:32:02] 'test' errored after 5.06 s
[13:32:02] Error in plugin 'gulp-jasmine'
Message:
    Tests failed
~Resolved~

I made a simple repo to reproduce the case: IdeaHunter/webworker-promises. Any help would appreciated

Right now im going to play with MakeCallback

I'm having a similar issue with node-argon2, the promise hangs indefinitely (even though it runs every single line of the HandleOKCallback). I pretty much based off your code.

@kkoopa

@kkoopa
The things going from strange to crazy when Im trying tweak code one way or another:

1. After reviewing docs on subject found that code might missing EscapableHandleScope since it creates the new promise object

void MyWorker::Run(const Nan::FunctionCallbackInfo<v8::Value>& info){
    Nan::EscapableHandleScope scope; 
    auto resolver = Promise::Resolver::New(info.GetIsolate());
    auto worker = new MyWorker();
    worker->SaveToPersistent("key",resolver);
    AsyncQueueWorker(worker);
    info.GetReturnValue().Set(scope.Escape(resolver->GetPromise()));
}

Result: test still failing

2. Make sure callbacks got called in js code

void MyWorker::HandleOKCallback(){
    Nan::HandleScope scope;
    Local<Value> argv[] = {
        New<Number>(42)
    };

    callback->Call(1, argv);
    res->Resolve(Nan::Undefined());
}

test(function (argument) {
    console.log(argument)
}).then(function (argument) {
     console.log('~Resolved~');
     done();
},done);

Result: Callback getting called

when running async worker~Test started~
42

    resolve the promise without jasmine timeout: failed

3. Trying to return promise from callback

void MyWorker::HandleOKCallback(){
    Nan::HandleScope scope;
    auto res =  GetFromPersistent("key").As<v8::Promise::Resolver>();
    Local<Value> argv[] = {
        res
    };

    callback->Call(1, argv);
    res->Resolve(Nan::Undefined());
}

Result: Promise returned as pening which is ok since we fired callback before promise resolved, test still failing

when running async worker~Test started~
Promise { <pending> }

    resolve the promise without jasmine timeout: failed

4. Firing callback after promise got resolved

res->Resolve(Nan::Undefined());
Local<Value> argv[] = {
    res
};
callback->Call(1, argv);

Result: Whops test get passed, but we did not do anything with promise except passing it to callback

when running async worker~Test started~
Promise { undefined }
~Resolved~

    resolve the promise without jasmine timeout: passed

5. Falling back to scenario (3) where we firing callback before promise got resolved, tweaking js code to check how does promise changed state after some timeout

test(function (argument) {
    console.log(argument)
    setTimeout(function() {console.log(argument)}, 10);
})

Result: Test passed. Wtf?

when running async worker~Test started~
Promise { <pending> }
Promise { undefined }
~Resolved~

    resolve the promise without jasmine timeout: passed

6. Fallback to (2) - stop passing promise to callback, keeping js code as (5):
Result: Test still passing. WTF?!!!

when running async worker~Test started~
42
42
~Resolved~

    resolve the promise without jasmine timeout: passed

Sorry for such a long message.
I have only one question - does it look like a bug?

@IdeaHunter

@IdeaHunter I fixed it using different v8 API, by creating a new (v8::)Context for resolving or rejecting promises. I can't confirm this is the solution but my tests not pass even using ES2015 async/await syntax.

Refactor

promise->Resolve(value);

to

promise->Resolve(Nan::New<v8::Context>(), value);

The same goes with reject.

I was not fully able to follow all of it, but now things are starting to look familiar. Will it still work if you remove the call to console.log? The reason I am asking is that console.log has a side-effect of kicking the next tick in the event queue. Moreover, Nan::Callback::Call makes use of Nan::MakeCallback, which makes use of node::MakeCallback. node::MakeCallback kicks the next tick in the event queue.

There is another function, Nan::Call, which takes a v8::Function and makes a direct v8::Function::Call, bypassing node::MakeCallback, thereby not kicking the next tick in the event queue. You can extract the v8::Function from a Nan::Callback through Nan::Callback::GetFunction, or its shorthand operator(*).

What happens if you repeat the relevant tests with the original JavaScript code, without the console.log and using Nan::Call instead of Nan::Callback::Call or Nan::MakeCallback? My hypothesis is that your promise will not get resolved under these circumstances. This would imply that promise resolution requires advancement of the event queue. If that is the case, it can be achieved in at least 2 (3) ways:

Make a callback to a dummy JavaScript function from C++, after you have resolved the Promise. This must go through node::MakeCallback, so use Nan::Callback::Call or Nan::MakeCallback. Its intended effect is to kick off the next tick.
Wrap the correct part of your JavaScript code in a setImmediate or setTimeout, again to advance the event queue.
(Have a useless call to console.log or some other function which has the side effect of advancing the event queue. This is probably the worst option and not really a solution.)

Essentially, your event loop is starved and you need to advance it to the next tick for promise resolution to take effect.

@kkoopa

@kkoopa
Yep if i just remove console.log without switching Nan::Call, code below still work - setTimeout is show-runner:

test(function (argument) {
    setTimeout(function() {}, 10);
}).then(function (argument) {
     done();
},done);

void MyWorker::HandleOKCallback(){
    Nan::HandleScope scope;
    auto res =  GetFromPersistent("key").As<v8::Promise::Resolver>();

    Local<Value> argv[] = {
        New<Number>(42)
    };
    callback->Call(1, argv);
    res->Resolve(Nan::Undefined());
}

And if i replace setTimeout with console.log it would stop working.
Another way to make things work is to remove setTimeout and reorder last 2 lines of c++ code as follow:

res->Resolve(Nan::Undefined());
callback->Call(1, argv);

Finally i tried suggested approach with both removing all console.log and using Nan::Call (took a while to figure out what second argument mean), code actually stopped working:

void MyWorker::HandleOKCallback(){
    Nan::HandleScope scope;
    auto res =  GetFromPersistent("key").As<v8::Promise::Resolver>();

    Local<Value> argv[] = {
        New<Number>(42)
    };
    res->Resolve(Nan::Undefined());
    Nan::Call(callback->GetFunction(),GetCurrentContext()->Global(),1, argv);//callback->Call(1, argv);
}

test(function (argument) {
     //console.log(argument);
}).then(function (argument) {
    done();
},done);

And if i return back console.log i can see it in output but code still not working

If got things right the observed behavior does not match one expected from event queue, otherwise adding console.log should made difference, right?

@ranisalt i tried you approach with no luck - my test still timeout ((

I don't know exactly where you have to have the event queue kicker nor exactly which is needed, console.log might not have the correct side effects for this, but it does not really matter since whatever side effects it has are unintentional anyway. However, I am now fairly certain the problem has been narrowed down to this.

I don't know if this is the best way of achieving what you need, it probably is not, but it could at least be used as a stopgap measure: Use NanCallback::Call or MakeCallback to any function after resolving your promise. This will advance the event queue. If you do not want to pass around an empty function for each call to your test method, do the following: Define an empty function in JavaScript, let's call it tickKicker, register it with your addon through an intialization function and store that in a static NanCallback, then call that function after resolving your promise. The result should be that the next tick is kicked and your thenable is called.

addon.initialize(function () {});
...
test().then(function (argument) { done(); }, done);

static NanCallback tickKicker;

NAN_METHOD(Initialize) {
  tickKicker.setFunction(info[0].As<v8::Function>());
}

NAN_METHOD(Test) {
...
}

void MyWorker::HandleOKCallback(){
  Nan::HandleScope scope;

  auto res = GetFromPersistent("key").As<v8::Promise::Resolver>();
  res->Resolve(Nan::Undefined());
  tickKicker();
}

On a side note, if you do not actually need string keys, it is more efficient to use numeric indices with AsyncWorker::GetFromPersistent and AsyncWorker::SaveToPersistent. However, index 0 is reserved for storing the callback function, so index 1 is the first available for general use.

UPDATE: The proper solution ought to be uv_prepare_t or uv_check_t https://groups.google.com/forum/#!topic/nodejs/vzH51rT3xbU

@kkoopa

@kkoopa thank you for help. It would be great solution for now since it give me a way to make things work right without hanging on them. In the mean time i would go to discuss with v8 maintainers since i believe the problem should have more generic solution. As for now im closing the issue since it is out of scope of Nan itself.

Ok. In case you missed the update I edited into my last reply, the proper solution would be to use uv_prepare_t or uv_check_t to make a nextTick from C++. Also note that the Promise API is marked experimental in v8.h.

@kkoopa

@kkoopa hm - not uv_prepare_t nor uv_check_t not even both together helping in my case. Also original thread suggesting the uv_async_send should be used before them - and i just checked nan.h and found only one reference of uv_async_send - and that one does't belong to AsyncWorker only to AsyncProgressWorker - can this be the case?

uv_async_send is (mostly) used to communicate between threads, one of which is the main thread (in which V8 runs). Nan::AsyncWorker::HandleOKCallback is already on the main thread, that is why V8 functions can be called from it, so there is no need to do an uv_async_send. That thread is quite old, so many things internal to node.js might have changed since. If it does not work, stick with the previously mentioned callback trick, which does have the desired effect.

Navigation Menu

Repository navigation

AsyncWorker & Promise - is it possible? #539

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

AsyncWorker & Promise - is it possible? #539

Description

Activity

kkoopa commented on Feb 3, 2016

addaleax commented on Feb 4, 2016

kkoopa commented on Feb 4, 2016

addaleax commented on Feb 4, 2016

IdeaHunter commented on Feb 4, 2016

ranisalt commented on Feb 13, 2016

IdeaHunter commented on Feb 22, 2016

ranisalt commented on Feb 22, 2016

kkoopa commented on Feb 22, 2016

IdeaHunter commented on Feb 22, 2016

kkoopa commented on Feb 22, 2016

IdeaHunter commented on Feb 22, 2016

kkoopa commented on Feb 22, 2016

IdeaHunter commented on Feb 22, 2016

kkoopa commented on Feb 22, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions