Repository navigation
Napi::Env::GetAndClearPendingException assumes exceptions are Error objects #912
Description
Activity
- changed the title
[-]Napi::Error definition not compliant with ECMAScript throw-ables[/-][+]Napi::Env::GetAndClearPendingException assumes exceptions are Error objects[/+]on Feb 20, 2021 Our C++-to-JS exception conversion code requires that exceptions be JS objects so we can keep persistent references to them (#31). We may need a workaround though.
One possible way forward as discussed in the @nodejs/node-api meeting is to provide a variant of
GetAndClearException()that returns aNapi::Valuerather than aNapi::Error, and only if C++ exception handling is turned off, making clear in the documentation the implications wrt.HandleScopeof not having a persistent reference.This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
After investigating the problem, I believe the best solution (not introduce any breaking change) will be:
- if the thrown value is primitive, we fetch the value and keep a "reference" by value, and convert them back to JavaScript once the value is requested.
- if the thrown value is
object, orfunction, just do what we are doing right now.
The key point is to prevent calling into JavaScript again in the exception handling to prevent from causing the VM state to be changed.
However, the (1) can apply on
null,undefined,boolean,number,string,bigint, but not onsymbol.symbols are unique values, we can not fetch the native value of thesymbolnor can we convert some native representative back tosymbol.A TC39 stage 2 proposal Symbols as WeakMap keys is trying to loosen the
WeakMapkeys type constraint to allowsymbolasWeakMapkeys, which is very similar to the case we are facing (somewhat). I opened nodejs/node#39131 to discuss losing the constraint on node-api.As discussed in today's Node-API meeting, we need to determine if various JavaScript engines allow creating references on primitive values. If this is the case, then the fix can be done in core via allowing references on all types. Otherwise, this will need fixes in node-addon-api instead.
Engine Allows References on Primitives Notes V8 Apple Yes See JSValueProtect, allows referencing any JSValue Firefox/Spidermonkey Yes See JS::PersistentRooted. JerryScript Yes See Reference counting. The example creates a reference on a primitive ( 3.14)React Native / Hermes @vmoroz can you help confirm whether or not React Native/Hermes supports creating references on primitive values?
The JSI in ReactNative/Hermes does not differentiate between local and referenced values. All values are either references (object/string/symbol) or C++ primitives such as number/bool. The jsi::Value that can be of any JS type is defined as a union: https://github.com/facebook/react-native/blob/034c6dfe34d240cf7c6314e767716317fa554351/ReactCommon/jsi/jsi/jsi.h#L938 .
I.e. there are no references for number/bool, but strings and symbols are always references.To implement JSI adapter for the NAPI I had to introduce a new type napi_ext_ref to work around the limitations of the NAPI references. It was mostly needed to support strings/symbols, to do better ref counting (auto-delete if ref count zero), and to do better weak ref counting (there can be multiple weak refs with their own ref count).
I would suggest that NAPI should adopt an improved reference type that feels and behaves similar to std::shared_ptr/std::weak_ptr from the C++ standard library.
@vmoroz I think we asked the wrong question last time. Based on discussion in the meeting today what we were trying to ask was:
@vmoroz can you help confirm whether or not React Native/Hermes supports creating persistent references on primitive values? In V8 references are either local or persistent and we get references for both primitive and object values. To be able to keep a reference outside of a specific scope we need to create persistent references in both cases. We want to make sure we can do the equivalent of creating a persistent reference for both primitive and object values in React Native/Hermes.
@mhdawson, yes, Hermes internally seems to have a mechanism creating persistent references for both primitive and object values. It can be done by providing a GC callback that reports root values. It is used in the JSI API implementation used by React Native. See https://github.com/facebook/hermes/blob/f3421c66a052bb12f400d527b858f4208b827c8b/API/hermes/hermes.cpp#L304
Though the JSI does not expose the references to numbers, bools, null, and undefined. These are converted from C++ values: https://github.com/facebook/hermes/blob/f3421c66a052bb12f400d527b858f4208b827c8b/API/hermes/hermes.cpp#L673
I am starting to look at the implementation of NAPI for Hermes, and hope to find soon if there any limitations with exposing references to numbers, bools, undefined, and null.
We discussed in today's meeting, for this particular application of throwing exceptions of primitive types: we really only need Symbol support from the engine, as we can handle the remaining primitives in the library (by taking the native primitive value and converting to/from JS values). It sounds like Hermes supports Symbol references so we should be able to move forward with this approach and lift the "only Function or Object" restriction on references to include Symbols.
Reacted by Michael DawsonNext step is for somebody to open a PR in core to extend support to Symbol. Effectively it should just be no longer blocking it on Symbols.
Change in Node-addon api
- In inline Error::Error(napi_env env, napi_value value)
- If created reference fails, create wrapper object, store value on object, create reference on that object
- In code that throws Error as JavaScript (ThrowAsJavaScriptException()), pull value off object if we created it and throw that
- Override Value() on Error so that it it does unwrap as well
- Document that if you cast the Error to a C ref then you get the wrapper object if it was primitive (and possibly symbol) and how you can get the inner value.
Hey @mhdawson , I just have some question with regard to implementing this change. Inside the
ThrowAsJavaScriptExceptionfunction, there's a call tonapi_throwusing theValue()func inherited from the Reference class. Could we leave the code insideThrowAsJavaScriptExceptionas is and simply override the Value() on error to do the unwrapping?Also it looks like the
Value()function from the base Reference class is not virtual, so what would the best way to override this function method from the Error class?Thanks!
@mhdawson When I tried to mark Value as virtual from the Reference base class, I got an error saying
After doing some research it seems to suggest that to do this we have to mark the ObjectReference dtor as a virtual function as well. I am not too sure but I feel like this would require us to restructure a lot of unrelated code within napi.h. What I am doing at the moment is to "override" the Value function by defining it inside the Error class, which seems to work. (#1075) Thanks!This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.
On common checks on each node-api calls in node-addon-api (e.g.
NAPI_THROW_IF_FAILED),Napi::Error::New(env)is used to construct proper error representative types to indicate the exception value. In https://github.com/nodejs/node-addon-api/blob/main/napi.h#L1354Napi::ErrorextendsNapi::ObjectReference, which in the term of itself is correct, JavaScript errors are objects. However, exceptions are not, they can be any JavaScript values like string, or number, although the meaning might not be clear when throwing primitives like numbers but it is still valid JavaScript codes.Thus, in the case of throwing non-object and non-function types between the border of node-addon-api and javascript (or using napi_throw values directly in c++ land), node-addon-api complains (fatal error) that
Napi::Errorcannot be constructed with non-objects and non-functions.example of calling into the addon:
This is what we will get: