From 40bcb09e6b82e7a1164cb3de56cb503d9b5a3d37 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 14 Jun 2024 08:07:44 -0700 Subject: [PATCH] src: restore ability to run under NAPI_EXPERIMENTAL (#1409) * 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`. * propagate correlation between version and flag to test addon build * remove duplicate yaml key --- .github/workflows/ci-win.yml | 7 +- .github/workflows/ci.yml | 12 ++- common.gypi | 1 + napi-inl.h | 166 +++++++++++++++++++------------ test/addon_build/tpl/binding.gyp | 1 + 5 files changed, 123 insertions(+), 64 deletions(-) diff --git a/.github/workflows/ci-win.yml b/.github/workflows/ci-win.yml index 6ceeb349d..70095021f 100644 --- a/.github/workflows/ci-win.yml +++ b/.github/workflows/ci-win.yml @@ -12,13 +12,16 @@ jobs: test: timeout-minutes: 30 strategy: + fail-fast: false matrix: - node-version: [ 18.x, 20.x, 21.x, 22.x ] + node-version: + - 18.x + - 20.x + - 22.x architecture: [x64, x86] os: - windows-2019 - windows-2022 - fail-fast: false # Don't cancel other jobs when one job fails runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 021adcac8..4c8b63377 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,15 @@ jobs: test: timeout-minutes: 30 strategy: + fail-fast: false matrix: - node-version: [ 18.x, 20.x, 21.x, 22.x ] + api_version: + - standard + - experimental + node-version: + - 18.x + - 20.x + - 22.x os: - macos-latest - ubuntu-latest @@ -43,6 +50,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 1e2faf5c2..9211ef926 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -31,6 +31,23 @@ namespace details { // Node.js releases. Only necessary when they are used in napi.h and napi-inl.h. constexpr int napi_no_external_buffers_allowed = 22; +#if (defined(NAPI_EXPERIMENTAL) && \ + defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER)) +template +inline void PostFinalizerWrapper(node_api_nogc_env nogc_env, + void* data, + void* hint) { + napi_status status = node_api_post_finalizer(nogc_env, finalizer, data, hint); + NAPI_FATAL_IF_FAILED( + status, "PostFinalizerWrapper", "node_api_post_finalizer failed"); +} +#else +template +inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) { + finalizer(env, data, hint); +} +#endif + template inline void default_finalizer(napi_env /*env*/, void* data, void* /*hint*/) { delete static_cast(data); @@ -65,7 +82,8 @@ inline napi_status AttachData(napi_env env, } } #else // NAPI_VERSION >= 5 - status = napi_add_finalizer(env, obj, data, finalizer, hint, nullptr); + status = napi_add_finalizer( + env, obj, data, details::PostFinalizerWrapper, hint, nullptr); #endif return status; } @@ -1777,7 +1795,8 @@ inline External External::New(napi_env env, napi_status status = napi_create_external(env, data, - details::FinalizeData::Wrapper, + details::PostFinalizerWrapper< + details::FinalizeData::Wrapper>, finalizeData, &value); if (status != napi_ok) { @@ -1800,7 +1819,8 @@ inline External External::New(napi_env env, napi_status status = napi_create_external( env, data, - details::FinalizeData::WrapperWithHint, + details::PostFinalizerWrapper< + details::FinalizeData::WrapperWithHint>, finalizeData, &value); if (status != napi_ok) { @@ -1913,7 +1933,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env, env, externalData, byteLength, - details::FinalizeData::Wrapper, + details::PostFinalizerWrapper< + details::FinalizeData::Wrapper>, finalizeData, &value); if (status != napi_ok) { @@ -1938,7 +1959,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env, env, externalData, byteLength, - details::FinalizeData::WrapperWithHint, + details::PostFinalizerWrapper< + details::FinalizeData::WrapperWithHint>, finalizeData, &value); if (status != napi_ok) { @@ -2655,13 +2677,14 @@ inline Buffer Buffer::New(napi_env env, details::FinalizeData* finalizeData = new details::FinalizeData( {std::move(finalizeCallback), nullptr}); - napi_status status = - napi_create_external_buffer(env, - length * sizeof(T), - data, - details::FinalizeData::Wrapper, - finalizeData, - &value); + napi_status status = napi_create_external_buffer( + env, + length * sizeof(T), + data, + details::PostFinalizerWrapper< + details::FinalizeData::Wrapper>, + finalizeData, + &value); if (status != napi_ok) { delete finalizeData; NAPI_THROW_IF_FAILED(env, status, Buffer()); @@ -2684,7 +2707,8 @@ inline Buffer Buffer::New(napi_env env, env, length * sizeof(T), data, - details::FinalizeData::WrapperWithHint, + details::PostFinalizerWrapper< + details::FinalizeData::WrapperWithHint>, finalizeData, &value); if (status != napi_ok) { @@ -2723,13 +2747,14 @@ inline Buffer Buffer::NewOrCopy(napi_env env, {std::move(finalizeCallback), nullptr}); #ifndef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED napi_value value; - napi_status status = - napi_create_external_buffer(env, - length * sizeof(T), - data, - details::FinalizeData::Wrapper, - finalizeData, - &value); + napi_status status = napi_create_external_buffer( + env, + length * sizeof(T), + data, + details::PostFinalizerWrapper< + details::FinalizeData::Wrapper>, + finalizeData, + &value); if (status == details::napi_no_external_buffers_allowed) { #endif // NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED // If we can't create an external buffer, we'll just copy the data. @@ -2762,7 +2787,8 @@ inline Buffer Buffer::NewOrCopy(napi_env env, env, length * sizeof(T), data, - details::FinalizeData::WrapperWithHint, + details::PostFinalizerWrapper< + details::FinalizeData::WrapperWithHint>, finalizeData, &value); if (status == details::napi_no_external_buffers_allowed) { @@ -3057,7 +3083,12 @@ 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 @@ -4431,7 +4462,12 @@ inline ObjectWrap::ObjectWrap(const Napi::CallbackInfo& callbackInfo) { napi_status status; napi_ref ref; T* instance = static_cast(this); - status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref); + status = napi_wrap(env, + wrapper, + instance, + details::PostFinalizerWrapper, + nullptr, + &ref); NAPI_THROW_IF_FAILED_VOID(env, status); Reference* instanceRef = instance; @@ -5327,19 +5363,21 @@ TypedThreadSafeFunction::New( auto* finalizeData = new details:: ThreadSafeFinalize( {data, finalizeCallback}); - napi_status status = napi_create_threadsafe_function( - env, - nullptr, - nullptr, - String::From(env, resourceName), - maxQueueSize, - initialThreadCount, - finalizeData, + auto fini = details::ThreadSafeFinalize:: - FinalizeFinalizeWrapperWithDataAndContext, - context, - CallJsInternal, - &tsfn._tsfn); + FinalizeFinalizeWrapperWithDataAndContext; + napi_status status = + napi_create_threadsafe_function(env, + nullptr, + nullptr, + String::From(env, resourceName), + maxQueueSize, + initialThreadCount, + finalizeData, + fini, + context, + CallJsInternal, + &tsfn._tsfn); if (status != napi_ok) { delete finalizeData; NAPI_THROW_IF_FAILED( @@ -5371,19 +5409,21 @@ TypedThreadSafeFunction::New( auto* finalizeData = new details:: ThreadSafeFinalize( {data, finalizeCallback}); - napi_status status = napi_create_threadsafe_function( - env, - nullptr, - resource, - String::From(env, resourceName), - maxQueueSize, - initialThreadCount, - finalizeData, + auto fini = details::ThreadSafeFinalize:: - FinalizeFinalizeWrapperWithDataAndContext, - context, - CallJsInternal, - &tsfn._tsfn); + FinalizeFinalizeWrapperWithDataAndContext; + napi_status status = + napi_create_threadsafe_function(env, + nullptr, + resource, + String::From(env, resourceName), + maxQueueSize, + initialThreadCount, + finalizeData, + fini, + context, + CallJsInternal, + &tsfn._tsfn); if (status != napi_ok) { delete finalizeData; NAPI_THROW_IF_FAILED( @@ -5487,19 +5527,21 @@ TypedThreadSafeFunction::New( auto* finalizeData = new details:: ThreadSafeFinalize( {data, finalizeCallback}); - napi_status status = napi_create_threadsafe_function( - env, - callback, - nullptr, - String::From(env, resourceName), - maxQueueSize, - initialThreadCount, - finalizeData, + auto fini = details::ThreadSafeFinalize:: - FinalizeFinalizeWrapperWithDataAndContext, - context, - CallJsInternal, - &tsfn._tsfn); + FinalizeFinalizeWrapperWithDataAndContext; + napi_status status = + napi_create_threadsafe_function(env, + callback, + nullptr, + String::From(env, resourceName), + maxQueueSize, + initialThreadCount, + finalizeData, + fini, + context, + CallJsInternal, + &tsfn._tsfn); if (status != napi_ok) { delete finalizeData; NAPI_THROW_IF_FAILED( @@ -5533,6 +5575,9 @@ TypedThreadSafeFunction::New( auto* finalizeData = new details:: ThreadSafeFinalize( {data, finalizeCallback}); + auto fini = + details::ThreadSafeFinalize:: + FinalizeFinalizeWrapperWithDataAndContext; napi_status status = napi_create_threadsafe_function( env, details::DefaultCallbackWrapper< @@ -5544,8 +5589,7 @@ TypedThreadSafeFunction::New( maxQueueSize, initialThreadCount, finalizeData, - details::ThreadSafeFinalize:: - FinalizeFinalizeWrapperWithDataAndContext, + fini, context, CallJsInternal, &tsfn._tsfn); diff --git a/test/addon_build/tpl/binding.gyp b/test/addon_build/tpl/binding.gyp index aa26f1acb..448fb9465 100644 --- a/test/addon_build/tpl/binding.gyp +++ b/test/addon_build/tpl/binding.gyp @@ -9,6 +9,7 @@ }, 'conditions': [ ['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ], + ['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ], ['disable_deprecated=="true"', { 'defines': ['NODE_ADDON_API_DISABLE_DEPRECATED'] }],