N-API request: Ability to report fatal exception resulting from callbacks that are thrown outside the regular JS call stack

[rolftimmermans]

I am attempting to (re)write a node module to use N-API, but there is an issue that I cannot see a workaround for.

I want to use uv_poll_t/uv_poll_init_socket etc. to set up socket polling to handle socket events in the native addon. In the native callback there is a code path that executes a JS callback. Looks something like this:

// env is stored in data field of the uv_poll_t handle
Napi::HandleScope scope(env);
Value().Get("emit").As<Napi::Function>().Call(Value(), {
    Napi::String::New(env, "message"),
    msg.ToBuffer(env),
});

Unfortunately any exception thrown in the JS callback will crash the process. Instead I want to report a fatal exception to Node, so that it can be delegated to the uncaughtException event of process.

Essentially, I want to use this code, identical to what is used in the async callback:

node/src/node_api.cc

Lines 3296 to 3301 in a10856a

	if (!env->last_exception.IsEmpty()) {
	v8::TryCatch try_catch(env->isolate);
	env->isolate->ThrowException(
	v8::Local<v8::Value>::New(env->isolate, env->last_exception));
	node::FatalException(env->isolate, try_catch);
	}

But there seems to be no way to do this. Even if I include v8.h and node.h I cannot convert the exception from N-API back to a v8 exception so I can call node::FatalException.

Ideally I'd like either:

1. Ability to call node::FatalException() via a supported addition to N-API (so that I can cause an uncaughtException event to be emitted); or

2. Ability to use uv_poll_t/uv_poll_init_socket etc via a supported addition to N-API, similar to napi_create_async_work etc.

[jasongin]

There is a napi_fatal_error() API that was added recently. It hasn't been added to the C++ wrapper yet, but meanwhile you should be able to call the C function. Does that meet your needs?

[rolftimmermans]

Unfortunately it doesn't seem like it would fit my needs. napi_fatal_error() calls node::FatalError() which prints a message and aborts. I want the uncaughtException handler to be invoked, which seems to be done by node::FatalException().

Having a similar wrapper for node::FatalException(), for example napi_fatal_exception() would work (although honestly the two function names would be easily confused).

[bnoordhuis]

What happens when you use napi_make_callback() instead of Function::Call()?

[rolftimmermans]

It seems the stack trace and abort is the same:

FATAL ERROR: Error::New napi_get_last_error_info
 1: node::Abort() [/usr/local/Cellar/node/8.4.0/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/usr/local/Cellar/node/8.4.0/bin/node]
 3: node::OnFatalError(char const*, char const*) [/usr/local/Cellar/node/8.4.0/bin/node]
 4: napi_create_function [/usr/local/Cellar/node/8.4.0/bin/node]
 5: Napi::Error::Fatal(char const*, char const*) [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
 6: Napi::Error::New(napi_env__*) [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
 7: Napi::Function::Call(napi_value__*, unsigned long, napi_value__* const*) const [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
 8: Napi::Function::Call(napi_value__*, std::initializer_list<napi_value__*> const&) const [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
 9: zmq::Socket::RecvCallback() [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
10: zmq::Socket::PollCallback(uv_poll_s*, int, int) [/Users/rolftimmermans/Code/vendor/zmq2/build/Debug/zmq.node]
11: uv__io_poll [/usr/local/Cellar/node/8.4.0/bin/node]
12: uv_run [/usr/local/Cellar/node/8.4.0/bin/node]
13: node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) [/usr/local/Cellar/node/8.4.0/bin/node]
14: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/usr/local/Cellar/node/8.4.0/bin/node]
15: node::Start(int, char**) [/usr/local/Cellar/node/8.4.0/bin/node]
16: start [/usr/local/Cellar/node/8.4.0/bin/node]

But looking at that node::FatalException is already called. Maybe I misunderstand how to properly call back to Javascript from uv_poll_init_socket/uv_poll_start?

[rolftimmermans]

Upon further investigation it seems it is broken for napi_create_async_work too. I guess I might be waiting for #14697.

[rolftimmermans]

#14697 solves a lot of problems, but the process uncaught exception handler is still not called by napi_make_callback.

[rolftimmermans]

I have found a solution/workaround/hack.

The following patch solves my problems

--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -2834,20 +2834,22 @@ napi_status napi_make_callback(napi_env env,
   v8::Local<v8::Function> v8func;
   CHECK_TO_FUNCTION(env, v8func, func);
 
   node::async_context* node_async_context =
     reinterpret_cast<node::async_context*>(async_context);
   if (node_async_context == nullptr) {
     static node::async_context empty_context = { 0, 0 };
     node_async_context = &empty_context;
   }
 
+  node::CallbackScope scope(isolate, v8recv, *node_async_context);
+
   v8::MaybeLocal<v8::Value> callback_result = node::MakeCallback(
       isolate, v8recv, v8func, argc,
       reinterpret_cast<v8::Local<v8::Value>*>(const_cast<napi_value*>(argv)),
       *node_async_context);
   CHECK_MAYBE_EMPTY(env, callback_result, napi_generic_failure);
 
   if (result != nullptr) {
     *result = v8impl::JsValueFromV8LocalValue(
         callback_result.ToLocalChecked());
   }

I really don't know whether or not this makes sense, but hopefully it is useful for someone more knowledgeable about the internals.

[jasongin]

@rolftimmermans I think that's probably the correct fix.
@addaleax Can you comment on this? You're more familiar with the details of CallbackScope and MakeCallback() than I am.

[addaleax]

I think that's probably the correct fix.

Not really … MakeCallback is basically CallbackScope + a function call, so you probably don’t want to do MakeCallbacks inside a CallbackScope (or at least that’s redundant). But more importantly, if you do it this way, you won’t get to catch exceptions thrown by MakeCallback if you actually do want to handle them.

(The origin of the reported issue here is that NAPI_PREAMBLE always sets up a try/catch block; and I don’t think we can disable that without breaking more code.)

So, ugh. This is not trivial. The most natural solution might just be to parametrize NAPI_PREAMBLE so that it can also create a verbose TryCatch, but that comes with significant API churn.

After that, maybe we could expose something that leads to the same code paths that Node’s own native uncaught exception handler also calls. I am not sure, but just adding a function that throws some input parameter as an exception inside a verbose TryCatch might work.

(And while I’m writing this: I’m not understanding myself rn why napi_throw works? It does use NAPI_PREAMBLE, so it looks like all it’s doing is throwing an exception inside a TryCatch …?)

[jasongin]

I’m not understanding myself rn why napi_throw works? It does use NAPI_PREAMBLE, so it looks like all it’s doing is throwing an exception inside a TryCatch …?

The exception thrown in napi_throw is caught by the TryCatch at that point, and saved in env->lastException. Then before the native callback returns, the env->lastException is rethrown so that it propagates out to the JavaScript caller.

I don't understand how a verbose TryCatch would help here: it would still allow an exception to result in a crash instead of being reported via the uncaught-exception handler, right?

It looks like we just need to add a napi_fatal_exception() API. We had intended to support custom async handling without using the napi_async_work APIs, which are just meant to be a convenience for the most common pattern. Without the ability to throw a fatal exception, it isn't possible to fully replicate the correct async behavior.

[addaleax]

Ooh, I missed that TryCatch in node_api.cc isn’t v8::TryCatch. Sorry.

I don't understand how a verbose TryCatch would help here: it would still allow an exception to result in a crash instead of being reported via the uncaught-exception handler, right?

As far as I understand it, the difference is that a verbose TryCatch would report the exception from the engine as if it was an uncaught one. I think that would work, or at least I can’t remember anything that would stand in the way of it.

(Which can also be in the form of an napi_fatal_exception() API, of course.)

[mafintosh]

Just ran into this same issue, anyway I can help get this fixed?

[mhdawson]

Working woth the test case in nodejs/node-addon-api#235, I'm thinking I'll add

NAPI_EXTERN napi_status napi_throw_fatal(napi_env env, napi_value error);

[mafintosh]

@mhdawson #19337

[mhdawson]

Believe this was closed by #19337, please re-open if you believe that is not the case.