Promises with AsyncWorker do not resolve properly

[ranisalt]

This is a followup of #539 since the discussion there went far away. I tried to isolate the problem where a promise resolver would not work correctly sometimes, and @julien-f reported in this issue the promise is not working properly when a timeout is set.

I created a minimal example with tests in this gist. Clone it, install it then test it and you'll see when promises are implemented with Nan the timeout expires.

From what I could understand from #539 it happens because setTimeout holds the tick until the timeout expires so the promise resolve or reject function only runs after the timeout expires and the next tick is kicked. From that gist, how should one proceed to fix this issue?

[kkoopa]

I don't know if it's V8 acting up or if it conformant to the specs, if it is Node + libuv acting up, or something else, but the exposed promise API is marked as experimental, so I would guess something is fishy in V8. It might be fixed in later versions for all I know. The only workaround I know of for now is to kick the event loop into the next tick somehow. One way is to somehow call node::MakeCallback after resolving the Promise, which will kick off the next tick.

[ranisalt]

I have wandered deep into the Lands of Tick Kicking and discovered there's a way to run pending tasks in the current context without forcing a global tick kicking. In fact, I just grabbed a line from inside the Environment::KickNextTick function and run it after resolving/rejecting the promise:

         promise->Resolve(Nan::GetCurrentContext(), Nan::New(value));
+        v8::Isolate::GetCurrent()->RunMicrotasks();

         promise->Reject(Nan::GetCurrentContext(), Nan::New(ErrorMessage()).ToLocalChecked());
+        v8::Isolate::GetCurrent()->RunMicrotasks();

Not sure if I'm lucky or this is correct, but it passes the test as you can check cloning the gist on the OP.

[kkoopa]

Ah, that seems like a very good option indeed. Much cleaner.

On March 3, 2016 5:50:26 AM GMT+02:00, Ranieri Althoff [email protected] wrote:

I have wandered deep into the Lands of Tick Kicking and discovered
there's a way to run pending tasks in the current context without
forcing a global tick kicking. In fact, I just grabbed a line from
inside the Environment::KickNextTick function and run it after
resolving/rejecting the promise:

        promise->Resolve(Nan::GetCurrentContext(), Nan::New(value));
+        v8::Isolate::GetCurrent()->RunMicrotasks();

promise->Reject(Nan::GetCurrentContext(),
Nan::New(ErrorMessage()).ToLocalChecked());
+        v8::Isolate::GetCurrent()->RunMicrotasks();

Not sure if I'm lucky or this is correct, but it passes the test as you
can check cloning the gist on the OP.

---

Reply to this email directly or view it on GitHub:
#541 (comment)

[ranisalt]

Ah, this does not work with ES7 async/await transpiled by babel somehow and is not working on a more complex example.

[vkurchatkin]

nodejs/node#5691

[Bamieh]

Anyone got a fix to this issue? its still happening to me. However I believe this is v8's responsibility to solve this not NAN,

[vkurchatkin]

You have to trigger microtasks manually. I guess Nan could implement a helper for that

[joeroback]

while this is still a problem for me too, it does not appear to be a problem within the context of the Atom editor.. I was prototyping some parts on the command line and was encountering this exact issue. When I finally moved my changes over into my Atom package I was working on, I did not need the hack to manually run the micro-tasks. I wonder is Atom is doing different.. Or if they are already working around the issue... because it does indeed seem and sound like a V8 issue..

[ranisalt]

Anyone got a fix to this issue?

If possible, handle Promises on pure JS. That's what I did, but I still hope for a better solution on the metal.

[enobufs]

Any progress on this issue? I am using node v8.9.4, AsyncWorker with v8::Promise on mocha not working for me either. The v8::Isolate::GetCurrent()->RunMicrotasks(); works for me too but not sure how safe that is...

[ranisalt]

@enobufs AFAIK nothing has advanced in that matter. I am using Promises on JS and passing the callbacks as arguments into C++ code instead, though I'd prefer a thinner JS layer.

[enobufs]

@ranisalt That's a bummer... The native promise seems to be working fine, so I guess something is missing here. Yeah, promisification at JS land seems to be the way to go at least for now.

[ranisalt]

I've tried to fix deprecation warnings on Nan::MakeCallback replacing with proper AsyncResource calling:

diff --git a/src/argon2.cpp b/src/argon2.cpp
index b950e01..615cd3f 100644
--- a/src/argon2.cpp
+++ b/src/argon2.cpp
@@ -61,7 +61,7 @@ argon2_context make_context(char* buf, const std::string& plain,
 class HashWorker final: public Nan::AsyncWorker {
 public:
     HashWorker(std::string plain, Options options) :
-        Nan::AsyncWorker{nullptr},
+        Nan::AsyncWorker{nullptr, "argon2:HashWorker"},
         plain{std::move(plain)},
         options{std::move(options)}
     {}
@@ -96,11 +96,15 @@ public:
     {
         Nan::HandleScope scope;
 
-        v8::Local<v8::Value> argv[] = {
+        auto argv = new v8::Local<v8::Value>[1] {
             Nan::CopyBuffer(output.data(), output.size()).ToLocalChecked()
         };
-        Nan::MakeCallback(GetFromPersistent(CONTEXT).As<v8::Object>(),
-            GetFromPersistent(RESOLVE).As<v8::Function>(), 1, argv);
+        Nan::Callback(GetFromPersistent(RESOLVE).As<v8::Function>()).Call(
+                /*target =*/ GetFromPersistent(CONTEXT).As<v8::Object>(),
+                /*argc =*/ 1, /*argv =*/ argv,
+                /*async_resource =*/ async_resource);
     }
 
     void HandleErrorCallback() override
@@ -108,9 +112,15 @@ public:
         Nan::HandleScope scope;
 
-        v8::Local<v8::Value> argv[] = {Nan::New(ErrorMessage()).ToLocalChecked()};
-        Nan::MakeCallback(GetFromPersistent(CONTEXT).As<v8::Object>(),
-                GetFromPersistent(REJECT).As<v8::Function>(), 1, argv);
+        auto argv = new v8::Local<v8::Value>[1] {
+            v8::Exception::Error(Nan::New(ErrorMessage()).ToLocalChecked())
+        };
+        Nan::Callback(GetFromPersistent(REJECT).As<v8::Function>()).Call(
+                /*target =*/ GetFromPersistent(CONTEXT).As<v8::Object>(),
+                /*argc =*/ 1, /*argv =*/ argv,
+                /*async_resource =*/ async_resource);
     }
 
-- 

It works as expected, but it's around 20% slower. Am I doing it right?

[kkoopa]

Why do you allocate argv on the heap? It is slow. It will also leak, unless you handle that somewhere else.

[ranisalt]

Why do you allocate argv on the heap? It is slow. It will also leak, unless you handle that somewhere else.

Because allocating on the stack causes destruction to happen before the callback is fired. IOW, I get segfaults. What do you suggest?

[kkoopa]

I assumed that it would work, since nothing I can think of has changed in that regard and I also assumed that you were doing a valid comparison. Based on the diff, you did not previously heap allocate it, so then it means it was wrong before, right?

First you call Callback::Call https://github.com/nodejs/nan/blob/master/nan.h#L1637-L1651

This calls Callback::Call_ https://github.com/nodejs/nan/blob/master/nan.h#L1674-L1685

which calls AsyncResource::runInAsyncScope https://github.com/nodejs/nan/blob/master/nan.h#L517-L528

which calls node::MakeCallback. There is no deferred execution until then, so if it previously worked without heap allocation, and it was correct, I would assume that it should also do so now.
Otherwise you are comparing the performance of something that does not work with something that does function properly.

[LubosD]

This little detail about RunMicrotasks() should be better documented. This is something anyone passing callbacks from native code into Node.js will encounter eventually.

When Googling, people find that they should use uv_async_send() to have their code run on the main thread, only to later find that it messes up Promises.

[bnoordhuis]

Node.js calls RunMicrotasks() automatically, there should be no need to call it manually.

More specifically: any time JS land is entered or exited, pending microtasks (promises) and process.nextTick() callbacks run.

The one exception is when you use v8::Function::Call() but don't do that, it's a bad idea for reasons beyond promises.

@kkoopa Anything left to do or can this be closed out?

[LubosD]

@bnoordhuis So what is the right way to call a JS function from C++? Because pretty much everywhere I see only v8::Function::Call() being suggested.

[bnoordhuis]

@LubosD Use Nan::MakeCallback().

[kkoopa]

@kkoopa Anything left to do or can this be closed out?

Nothing I can think of. Note that v8::Function::Call should never be used directly, it should be either Nan::Callor Nan::MakeCallback depending on context.

[bnoordhuis]

Okay, I'll close this then.

[ranisalt]

I think that last conversation is not on the same scope as the issue. I asked specifically about promise resolving/rejecting, which seems not to work as a function internally but rather a Resolver object with some methods.

Maybe there could be a wrapper around a promise resolver that flushes microtask queue or whatever? The gist code still does not work without some v8 stuff that can change.

[bnoordhuis]

Maybe there could be a wrapper around a promise resolver that flushes microtask queue or whatever?

Not until nan drops support for Node.js < v4.x.

[ranisalt]

Will it happen someday? Those versions have already EOLd long ago, and nan is one of the libraries that can help pushing forward.

[bnoordhuis]

Undoubtedly. See discussion in #676.