Skip to content

Commit

Permalink
src: restore ability to run under NAPI_EXPERIMENTAL
Browse files Browse the repository at this point in the history
Since we made the default for Node.js core finalizers synchronous for
users running with `NAPI_EXPERIMENTAL` and introduced
`env->CheckGCAccess()` in Node.js core, we must now defer all
finalizers in node-addon-api, because our users likely make
non-gc-safe Node-API calls from existing finalizers. To that end,

  * Use the NAPI_VERSION environment variable to detect whether
    `NAPI_EXPERIMENTAL` should be on, and add it to the defines if
    `NAPI_VERSION` is set to `NAPI_VERSION_EXPERIMENTAL`, i.e.
    2147483647.
  * When building with `NAPI_EXPERIMENTAL`,
    * render all finalizers asynchronous, and
    * expect `napi_cannot_run_js` instead of `napi_exception_pending`.
  • Loading branch information
gabrielschulhof committed Jan 26, 2024
1 parent 66e6e0e commit 8b19089
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 7 deletions.
6 changes: 6 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jobs:
timeout-minutes: 30
strategy:
matrix:
api_version:
- standard
- experimental
node-version: [ 18.x, 20.x, 21.x ]
os:
- macos-latest
Expand Down Expand Up @@ -43,6 +46,9 @@ jobs:
npm install
- name: npm test
run: |
if [ "${{ matrix.api_version }}" = "experimental" ]; then
export NAPI_VERSION=2147483647
fi
if [ "${{ matrix.compiler }}" = "gcc" ]; then
export CC="gcc" CXX="g++"
fi
Expand Down
1 change: 1 addition & 0 deletions common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
},
'conditions': [
['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ],
['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ],
['disable_deprecated=="true"', {
'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED']
}],
Expand Down
61 changes: 54 additions & 7 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,36 @@ napi_value TemplatedInstanceVoidCallback(napi_env env, napi_callback_info info)
});
}

inline void DeferredCallback(const char* placeOfFailure,
napi_env env,
void* data,
void* hint,
napi_finalize fini) {
#ifdef NAPI_EXPERIMENTAL
napi_status status = node_api_post_finalizer(env, fini, data, hint);
NAPI_FATAL_IF_FAILED(
status, placeOfFailure, "DeferredCallback -> node_api_post_finalizer");
#else
(void)placeOfFailure;
fini(env, data, hint);
#endif
}

template <typename T, typename Finalizer, typename Hint = void>
struct FinalizeData {
static inline void Wrapper(napi_env env,
void* data,
void* finalizeHint) NAPI_NOEXCEPT {
DeferredCallback("FinalizeData::Wrapper",
env,
data,
finalizeHint,
FinalizeData<T, Finalizer>::SyncWrapper);
}

static inline void SyncWrapper(napi_env env,
void* data,
void* finalizeHint) NAPI_NOEXCEPT {
WrapVoidCallback([&] {
FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
finalizeData->callback(Env(env), static_cast<T*>(data));
Expand All @@ -195,6 +220,16 @@ struct FinalizeData {
static inline void WrapperWithHint(napi_env env,
void* data,
void* finalizeHint) NAPI_NOEXCEPT {
DeferredCallback("FinalizeData::WrapperWithHint",
env,
data,
finalizeHint,
FinalizeData<T, Finalizer, Hint>::SyncWrapperWithHint);
}

static inline void SyncWrapperWithHint(napi_env env,
void* data,
void* finalizeHint) NAPI_NOEXCEPT {
WrapVoidCallback([&] {
FinalizeData* finalizeData = static_cast<FinalizeData*>(finalizeHint);
finalizeData->callback(
Expand Down Expand Up @@ -2731,7 +2766,7 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
#endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
// If we can't create an external buffer, we'll just copy the data.
Buffer<T> ret = Buffer<T>::Copy(env, data, length);
details::FinalizeData<T, Finalizer>::Wrapper(env, data, finalizeData);
details::FinalizeData<T, Finalizer>::SyncWrapper(env, data, finalizeData);
return ret;
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
}
Expand Down Expand Up @@ -2766,7 +2801,7 @@ inline Buffer<T> Buffer<T>::NewOrCopy(napi_env env,
#endif
// If we can't create an external buffer, we'll just copy the data.
Buffer<T> ret = Buffer<T>::Copy(env, data, length);
details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint(
details::FinalizeData<T, Finalizer, Hint>::SyncWrapperWithHint(
env, data, finalizeData);
return ret;
#ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED
Expand Down Expand Up @@ -3054,7 +3089,13 @@ inline void Error::ThrowAsJavaScriptException() const {

status = napi_throw(_env, Value());

if (status == napi_pending_exception) {
#ifdef NAPI_EXPERIMENTAL
napi_status expected_failure_mode = napi_cannot_run_js;
#else
napi_status expected_failure_mode = napi_pending_exception;
#endif

if (status == expected_failure_mode) {
// The environment must be terminating as we checked earlier and there
// was no pending exception. In this case continuing will result
// in a fatal error and there is nothing the author has done incorrectly
Expand Down Expand Up @@ -4879,10 +4920,16 @@ template <typename T>
inline void ObjectWrap<T>::FinalizeCallback(napi_env env,
void* data,
void* /*hint*/) {
HandleScope scope(env);
T* instance = static_cast<T*>(data);
instance->Finalize(Napi::Env(env));
delete instance;
details::DeferredCallback("ObjectWrap<T>::FinalizeCallback",
env,
data,
nullptr,
[](napi_env env, void* data, void* /*hint*/) {
HandleScope scope(env);
T* instance = static_cast<T*>(data);
instance->Finalize(Napi::Env(env));
delete instance;
});
}

template <typename T>
Expand Down

0 comments on commit 8b19089

Please sign in to comment.