Closed KevinEady closed 4 weeks ago
Currently, the four nogc methods call NAPI_THROW_IF_FAILED_VOID
, creating a new Napi::Error
. This won't work in nogc-land since it's creating a new Value. I would like to discuss this on the 24 May Node-API call.
Also struggling a bit with figuring out how to make napi[-inl].h
smart enough to know when node_api_nogc_env
is available for use. For example, we cannot use these new types if compiling with headers for node v18.17.1, and I don't think we want to break compilation on non-most-recent-Node.js builds.
Discussed in 24 May Node-API meeting:
nogc
functions could fail; if they are "reasonable" (eg. invalid parameters passed), we could just some fatal exception insteadAnother issue that i'm struggling with regarding ObjectWrap
. The destructor uses Reference<Napi::Object>::Value()
which calls napi_get_reference_value
, and this method cannot be used inside GC:
Thoughts on how to proceed in this use case...? Does this napi_remove_wrap
need to be moved into a post-finalizer block, scheduled/added at the ObjectWrap instance construction?
cc: @legendecas @gabrielschulhof @vmoroz
Additionally, in ObjectWrap construction, the napi_wrap
gets passed ObjectWrap<T>::FinalizeCallback
, which in turn creates some handle scopes, which cannot be done in finalization.
Not sure how to proceed on this either. https://github.com/nodejs/node-addon-api/blob/7c79c331405ffc41f8d5dab958cde3e93bf90306/napi-inl.h#L4878-L4886
So continuing to dig a bit, mostly keeping this as notes... The combination of the destructor ~ObjectWrap
and the FinalizeCallback
is supposed to behave in this manner:
virtual void Finalize(Env)
runs.So, there are two scenarios we need to cover:
ObjectWrap
'd instance inside a native C++ method on the stack that is never returned to Node, causing the deletion when the object goes out of scope.ObjectWrap
'd instance and returning it back to Node for ownership.Seems like this is an exploit of the behavior of napi_get_reference_value
. napi_get_reference_value
will fail if it is called in finalizers without opening a handle scope. However, in a finalizer, the reference was reset and napi_get_reference_value
will simply return a nullptr (ref).
napi_get_reference_value
will fail if it is called in finalizers without opening a handle scope.
From my understanding, this is intended behavior...? Since the function takes a napi_env
(vs. the nogc
counterpart), I expect that it requires some sort of JavaScript engine integration/usage.
Hopefully we can discuss this in today's meeting.
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.
Since https://github.com/nodejs/node/pull/50060 is now in core, we need to add support for these types.
Looking at the Node-API docs, we can find which node-addon-api methods on
Napi::Env
should be split between normal andnogc
variants. The normal variant can use nogc methods, but not the other way around:AddCleanupHook
GetInstanceData
SetInstanceData
GetModuleFileName
Global
Undefined
Null
IsExceptionPending
GetAndClearPendingException
RunScript
Additionally, all JavaScript-using paradigms (creating new Values, calling functions, ...) should use the normal variant, and have a compile-time error if a nogc variant is supplied.