From 08ce9722cd081d33684126934ce69fd00395cc11 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 1 Feb 2024 22:12:32 -0800 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 | 12 +- common.gypi | 1 + napi-inl.h | 166 +++++++++++------- .../threadsafe_function_existing_tsfn.cc | 2 +- ...typed_threadsafe_function_existing_tsfn.cc | 2 +- 5 files changed, 119 insertions(+), 64 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b217c6f2e..cd4841ffe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,7 +13,14 @@ jobs: timeout-minutes: 30 strategy: matrix: - node-version: [ 18.x, 20.x, 21.x, 22.x ] + api_version: + - standard + - experimental + node-version: + - 18.x + - 20.x + - 21.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 40e7d4208..c134b58b3 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; } @@ -1774,7 +1792,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) { @@ -1797,7 +1816,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) { @@ -1910,7 +1930,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) { @@ -1935,7 +1956,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) { @@ -2652,13 +2674,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()); @@ -2681,7 +2704,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) { @@ -2720,13 +2744,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. @@ -2759,7 +2784,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) { @@ -3054,7 +3080,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 @@ -4428,7 +4459,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; @@ -5324,19 +5360,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( @@ -5368,19 +5406,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( @@ -5484,19 +5524,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( @@ -5530,6 +5572,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< @@ -5541,8 +5586,7 @@ TypedThreadSafeFunction::New( maxQueueSize, initialThreadCount, finalizeData, - details::ThreadSafeFinalize:: - FinalizeFinalizeWrapperWithDataAndContext, + fini, context, CallJsInternal, &tsfn._tsfn); diff --git a/test/threadsafe_function/threadsafe_function_existing_tsfn.cc b/test/threadsafe_function/threadsafe_function_existing_tsfn.cc index 226493703..ea3241f51 100644 --- a/test/threadsafe_function/threadsafe_function_existing_tsfn.cc +++ b/test/threadsafe_function/threadsafe_function_existing_tsfn.cc @@ -86,7 +86,7 @@ static Value TestCall(const CallbackInfo& info) { 0, 1, nullptr, /*finalize data*/ - FinalizeCB, + TSFN_FINALIZER(FinalizeCB), testContext, hasData ? CallJSWithData : CallJSNoData, &testContext->tsfn); diff --git a/test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc b/test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc index daa273fcb..3c6a4aba5 100644 --- a/test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc +++ b/test/typed_threadsafe_function/typed_threadsafe_function_existing_tsfn.cc @@ -88,7 +88,7 @@ static Value TestCall(const CallbackInfo& info) { 0, 1, nullptr, /*finalize data*/ - FinalizeCB, + TSFN_FINALIZER(FinalizeCB), testContext, hasData ? CallJSWithData : CallJSNoData, &testContext->tsfn);