Repository navigation
Exception handling broken with instance accessors #621
Description
Hey folks,
While migrating Realm to NAPI we encountered this issue.
To demonstrate it I have forked the node-addon-examples and modified one of them here
https://github.com/blagoev/node-addon-examples/tree/master/6_object_wrap/node-addon-api
There is an issue in the example code, but mainly an issue with exception handling when instance accessors are invoked from C++. I tried to comment as much as possible.
#include "myobject.h"
Napi::FunctionReference MyObject::constructor;
Napi::Object MyObject::Init(Napi::Env env, Napi::Object exports) {
Napi::HandleScope scope(env);
Napi::Function func =
DefineClass(env,
"MyObject",
{InstanceMethod("plusOne", &MyObject::PlusOne),
InstanceAccessor("value", &MyObject::GetValue, nullptr),
InstanceMethod("multiply", &MyObject::Multiply),
});
constructor = Napi::Persistent(func);
constructor.SuppressDestruct();
exports.Set("MyObject", func);
return exports;
}
//This demonstrates mostly an error in the example code and not a bug in NAPI C++ addon code. Still I commented it as an issue
//Given the example has not enabled c++ exeptions, there should be a `return;` after the `ThrowAsJavaScriptException` statement
MyObject::MyObject(const Napi::CallbackInfo& info)
: Napi::ObjectWrap<MyObject>(info) {
Napi::Env env = info.Env();
Napi::HandleScope scope(env);
int length = info.Length();
if (length <= 0 || !info[0].IsNumber()) {
//This does not do `throw Napi::TypeError::New(env, "Number expected")
//It means the statements after this call will get executed. It's wrong for many reasons mostly because it brakes normal exception behaviour.
Napi::TypeError::New(env, "Number expected").ThrowAsJavaScriptException();
//With eneabled c++ exceptions this should be the correct way
//throw Napi::TypeError::New(env, "Number expected");
//With disabled c++ exceptions this should be the correct way
//return;
}
//When this executes the exception thrown to JS is "A number was expected" and not "Number expected" which was the intended behaviour
Napi::Number value = info[0].As<Napi::Number>();
this->value_ = value.DoubleValue();
}
//Lets make GetValue call multiply
Napi::Value MyObject::GetValue(const Napi::CallbackInfo& info) {
Napi::Function func = info.This().As<Napi::Object>().Get("multiply").As<Napi::Function>();
return func.Call(info.This(), { info.This() });
}
//Lets make PlusOne try to get the value before doing some work
Napi::Value MyObject::PlusOne(const Napi::CallbackInfo& info) {
Napi::Object object = info[0].As<Napi::Object>();
//this should fail with 'Not implemented' and throw the same exception to the caller
Napi::Value value = object.GetFixed("value");
//do some work with the value
//return the result
Napi::Value result = info.Env().Null();
return result;
}
//Multiply is not implemented right now
Napi::Value MyObject::Multiply(const Napi::CallbackInfo& info) {
throw Napi::Error::New(info.Env(), "Not implemented");
}and the JS code
var addon = require('bindings')('addon');
try {
var obj = new addon.MyObject();
}
catch (e) {
if (e.message != "Number expected") {
console.error("Incorrect expected exception message:" + e.message);
}
}
var obj = new addon.MyObject(10);
try {
//NAPI: should fail with `Not implemented` exception
obj.multiply();
}
catch(e) {
if (e.message != "Not implemented") {
throw new Error("Incorrect expected exception message: " + e.message);
}
}
try {
//NAPI: should fail with `Not implemented` exception
var val = obj.value;
}
catch(e) {
if (e.message != "Not implemented") {
throw new Error("Incorrect expected exception message: " + e.message);
}
}
try {
//NAPI: should fail with `Not implemented` exception.
var val = obj.plusOne(obj);
}
catch(e) {
if (e.message != "Not implemented") {
throw new Error("Incorrect expected exception message: " + e.message);
}
}
console.log("Success");Here is an example/possible fix to this issue
Change this in your napi-inl.h
//old implementation
//inline Value Object::Get(const char* utf8name) const {
// napi_value result;
// napi_status status = napi_get_named_property(_env, _value, utf8name, &result);
// NAPI_THROW_IF_FAILED(_env, status, Value());
// return Value(_env, result);
//}
//new implementation
inline Value Object::Get(const char* utf8name) const {
napi_value result;
napi_status status = napi_get_named_property(_env, _value, utf8name, &result);
bool pending;
napi_is_exception_pending(_env, &pending);
if (pending) {
napi_value error;
napi_get_and_clear_last_exception(_env, &error);
Napi::Error er = Napi::Error(_env, error);
throw er;
}
NAPI_THROW_IF_FAILED(_env, status, Value());
return Value(_env, result);
}I did not do a PR since this may be happening somewhere else and there may be some better way to do it which fixes multiple places
I would love to have this fixed upstream so we can skip forking napi-inl.h in our project.
cheers
Activity
objectwrap: forgot return statement
gabrielschulhof commentedon Dec 12, 2019
The problem is that, if
napi_status != napi_ok && napi_status != napi_pending_exception, although we check if there's an exception pending, we clear that exception and replace it with one generated by node-addon-api.If there's a pending exception we should always honour it and get back to the engine ASAP, no matter that the
napi_statuswas notnapi_pending_exception.Ideally we'd fix this in N-API core and always return
napi_pending_exceptionif a call into the engine has resulted in an exception, but alas, we have places where we don't do that – which is why we document that, although the return status may not benapi_pending_exception, if it's notnapi_ok, then an exception may always be pending.As a framework on top of N-API, I believe we can correct at the above-linked place what I believe is a mistake we made in N-API and treat any non-
napi_pending_exceptionreturn status that, nevertheless has an exception pending, asnapi_pending_exception.@nodejs/n-api @tniessen WDYT?
gabrielschulhof commentedon Dec 12, 2019
In the case of the chain of calls illustrated in @blagoev's code, the site of the mistake is
napi_get_named_property(), which returnsnapi_generic_errorif the maybe result of the get is empty. It should behave likenapi_call_function(), which only returnsnapi_generic_failureif the maybe result of the function call into JS is empty for a reason other than that of a pending exception.tniessen commentedon Dec 12, 2019
I agree that having the
napi_pending_exceptionerror code without using it where it would seem appropriate is contradictory, and I would also like to see it used more. If an exception may be pending regardless of whether the error code wasnapi_pending_exceptionor not, it does not seem like a helpful error code.At the same time, the N-API docs state very clearly how we should be handling other errors codes if there is a pending exception, and it seems that we don't do that correctly in node-addon-api, so we should fix that.
gabrielschulhof commentedon Dec 12, 2019
@tniessen IIRC we added the statement to the docs regarding non-
napi_pending_exceptiononly because we had learned that we were returning non-napi_pending_exceptionwhile there was a pending exception.I'm not sure how we should handle the situation where the return code is non-
napi_pending_exceptionbut there is an exception. Currently we replace the pending exception with a new one, swallowing the old one silently. The change I'm proposing is to not replace the exception with another one. Is there another possible change we can make? I mean, we could chain exceptions, by placing the old exception onto a property of the new one. Is that more useful? Is there any other change we can make to improve this scenario?tniessen commentedon Dec 12, 2019
I would expect this behavior: Do not replace the exception. If users want to chain exceptions, they can do that in their addon.
blagoev commentedon Dec 12, 2019
If I may express my opinion. I would expect the exception to be the same as if the code was called from JS. I tried to demonstrate that in the sample, but I think I should have been more clear on it. If you do
obj.valuefrom JS you will get the exception. And if this code is done in native it should behave the same. I guess its the same what @tniessen says above my comment. No chaining of exceptions is the expected way, just rethrow the existing exception.My way of thinking of doing this, or any other thing for that matter would be, if the code is written in JS only and if it is written in native only it should behave the same.
One way of defining a general approach is possibly this. If a
napi_call returns nonnapi_okresult and/or there is a pending exception thenapi_call is considered failed with the pending exception taking priority. If both are true, like the current issue, then maybe have the napi result as a property on the rethrown error object. But then I don't know how useful that would be and if rethrown twice it may get overwritten by different error resultcheers
objectwrap: forgot return statement (#123)
error: do not replace pending exception
error: do not replace pending exception
error: do not replace pending exception
error: do not replace pending exception
error: do not replace pending exception
error: do not replace pending exception