N-API stability issues?

[s-h-a-d-o-w]

https://github.com/s-h-a-d-o-w/napi-test

• Version: v10.3.0
• Platform: Win7 x64

My repo that I linked to above contains a very simple example based around Promises. Considering the fact that N-API in node 10 is called "Stable", I would've expected that at least once everything was compiled successfully, my code would run consistently. Unfortunately, it doesn't and I wonder whether it's N-API's fault or mine (Maybe some problem with how I handle the async work? That's the only thing I can think of that might cause random problems):

Expected output:

D:\...>node .
Before Promise resolved
Promise resolved!

Errors that I get about 10% of the time (One time, execution even froze, I had to interrupt it. And sometimes, there is no output at all...):

D:\...>node .
Before Promise resolved
FATAL ERROR: EscapableHandleScope::Escape Escape value set twice
 1: node::DecodeWrite
 2: node_module_register
 3: v8::EscapableHandleScope::Escape
 4: v8::Function::Call
 5: std::basic_ostream<char,std::char_traits<char> >::operator<<
 6: v8::internal::interpreter::BytecodeDecoder::Decode
 7: v8::internal::RegExpImpl::Exec
 8: v8::internal::RegExpImpl::Exec
 9: v8::internal::RegExpImpl::Exec
10: 000000CB3B484281

D:\...>node .
console.js:196
Console.prototype.log = function log(...args) {
                                    ^^

SyntaxError: Unexpected identifier
    at Object.<anonymous> (D:\development\github\mute-volume\something.js:10:9)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:238:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:572:3)

D:\...>node .
Stacktrace:
   ptr1=0000029304803B99
    ptr2=0000000000000000
    ptr3=0000000000000000
    ptr4=0000000000000000
    failure_message_object=00000000001C5C10

==== JS stack trace =========================================

    0: ExitFrame [pc: 0000018337E84281]
    1: StubFrame [pc: 0000018337F05787]
Security context: 0000029304820769 <JSObject>

[mhdawson]

I think the problem is that it is not safe to execute N-API calls in the execute callback because it does not run on the main thread.

Looking at the documentation I believe we need to make this much clearer. it does say that execute does not run on the main thread (see https://nodejs.org/api/n-api.html) where it includes:

[in] execute: The native function which should be called to execute the logic asynchronously. The given function is called from a worker pool thread and can execute in parallel with the main event loop thread.

.
Since it does not run on the main event loop thread, it is not necessarily safe to run JavaScript (or N-API calls which effectively do the same).

@gabrielschulhof do you have any other comments to add?

[s-h-a-d-o-w]

Ah, that's a detail I didn't really take note of, nor knew what it meant (it not being safe).

So - I would simply put the resolving of the deferred in complete instead and everything should be good, right? (I've just tried that and unfortunately, it doesn't work - while what I had was unstable, at least the .then() was run whenever execution was successful. But if I resolve the deferred in complete, the process exits before then() runs.)

[gabrielschulhof]

We really shouldn't have passed the `napi_env` into the execute callback, because it lends the expression that the full gamut of APIs which accept a `napi_env` is available – where in fact it is not.

…

On Tue, Jun 5, 2018 at 5:26 PM, Andreas Opferkuch ***@***.***> wrote: Ah, that's a detail I didn't really take not of, nor knew what it meant (it not being safe). So - I would simply put the resolving of the Promise in complete instead and everything should be good, right? (I've just tried that and unfortunately, it doesn't work - while what I had was unstable, at least the .then() was run whenever execution was successful. But if I resolve the Promise in complete, the process exists before then() runs.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1318 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0VWQCAvJxiMOSHBxlpFIvkiT6p48ks5t5veDgaJpZM4UZWUI> .

[mhdawson]

@gabrielschulhof I'll create a PR to clarify in the docs at least to start.

@s-h-a-d-o-w moving to complete is the right way to go. I wonder whether you are are not seeing the output because the console log is just before the exit. Can you try adding something else that keeps that the process alive beyond that point (for example using setTimeout?)

[s-h-a-d-o-w]

I've narrowed down the problem to the call to napi_delete_async_work (pushed to the repo). Execution interrupts exactly at that point.

[mhdawson]

You may now be running into nodejs/node#20966 (comment)

[s-h-a-d-o-w]

Ah, cool - well... you know... good to know. 😉

Thanks for all your help!