From 8b19089a995ce2ffb93c403cf0cf23c6731873e9 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 26 Oct 2023 22:14:04 -0700 Subject: [PATCH] src: restore ability to run under NAPI_EXPERIMENTAL 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`. --- .github/workflows/ci.yml | 6 ++++ common.gypi | 1 + napi-inl.h | 61 +++++++++++++++++++++++++++++++++++----- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f400cac9..a1eb0a061 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 @@ -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 diff --git a/common.gypi b/common.gypi index 06c0176b2..e594f14ff 100644 --- a/common.gypi +++ b/common.gypi @@ -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'] }], diff --git a/napi-inl.h b/napi-inl.h index a5ae7af72..097dfcc41 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -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 struct FinalizeData { static inline void Wrapper(napi_env env, void* data, void* finalizeHint) NAPI_NOEXCEPT { + DeferredCallback("FinalizeData::Wrapper", + env, + data, + finalizeHint, + FinalizeData::SyncWrapper); + } + + static inline void SyncWrapper(napi_env env, + void* data, + void* finalizeHint) NAPI_NOEXCEPT { WrapVoidCallback([&] { FinalizeData* finalizeData = static_cast(finalizeHint); finalizeData->callback(Env(env), static_cast(data)); @@ -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::SyncWrapperWithHint); + } + + static inline void SyncWrapperWithHint(napi_env env, + void* data, + void* finalizeHint) NAPI_NOEXCEPT { WrapVoidCallback([&] { FinalizeData* finalizeData = static_cast(finalizeHint); finalizeData->callback( @@ -2731,7 +2766,7 @@ inline Buffer Buffer::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 ret = Buffer::Copy(env, data, length); - details::FinalizeData::Wrapper(env, data, finalizeData); + details::FinalizeData::SyncWrapper(env, data, finalizeData); return ret; #ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED } @@ -2766,7 +2801,7 @@ inline Buffer Buffer::NewOrCopy(napi_env env, #endif // If we can't create an external buffer, we'll just copy the data. Buffer ret = Buffer::Copy(env, data, length); - details::FinalizeData::WrapperWithHint( + details::FinalizeData::SyncWrapperWithHint( env, data, finalizeData); return ret; #ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED @@ -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 @@ -4879,10 +4920,16 @@ template inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* /*hint*/) { - HandleScope scope(env); - T* instance = static_cast(data); - instance->Finalize(Napi::Env(env)); - delete instance; + details::DeferredCallback("ObjectWrap::FinalizeCallback", + env, + data, + nullptr, + [](napi_env env, void* data, void* /*hint*/) { + HandleScope scope(env); + T* instance = static_cast(data); + instance->Finalize(Napi::Env(env)); + delete instance; + }); } template