Exception handling broken with instance accessors

[blagoev]

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

[gabrielschulhof]

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_status was not napi_pending_exception.

Ideally we'd fix this in N-API core and always return napi_pending_exception if 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 be napi_pending_exception, if it's not napi_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_exception return status that, nevertheless has an exception pending, as napi_pending_exception.

@nodejs/n-api @tniessen WDYT?

[gabrielschulhof]

In the case of the chain of calls illustrated in @blagoev's code, the site of the mistake is napi_get_named_property(), which returns napi_generic_error if the maybe result of the get is empty. It should behave like napi_call_function(), which only returns napi_generic_failure if the maybe result of the function call into JS is empty for a reason other than that of a pending exception.

[tniessen]

I agree that having the napi_pending_exception error 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 was napi_pending_exception or 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]

@tniessen IIRC we added the statement to the docs regarding non-napi_pending_exception only because we had learned that we were returning non-napi_pending_exception while there was a pending exception.

I'm not sure how we should handle the situation where the return code is non-napi_pending_exception but 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]

The change I'm proposing is to not replace the exception with another one.

I would expect this behavior: Do not replace the exception. If users want to chain exceptions, they can do that in their addon.

[blagoev]

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.value from 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 non napi_ok result and/or there is a pending exception the napi_ 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 result

cheers