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
nodejs · Jun 14, 2024 · 40bcb09 · 40bcb09
1 parent 108e78b
commit 40bcb09
Show file tree

Hide file tree

Showing 5 changed files with 123 additions and 64 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 <napi_finalize finalizer>
+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 <napi_finalize finalizer>
+inline void PostFinalizerWrapper(napi_env env, void* data, void* hint) {
+  finalizer(env, data, hint);
+}
+#endif
+
 template <typename FreeType>
 inline void default_finalizer(napi_env /*env*/, void* data, void* /*hint*/) {
   delete static_cast<FreeType*>(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<finalizer>, hint, nullptr);
 #endif
   return status;
 }
@@ -1777,7 +1795,8 @@ inline External<T> External<T>::New(napi_env env,
   napi_status status =
       napi_create_external(env,
                            data,
-                           details::FinalizeData<T, Finalizer>::Wrapper,
+                           details::PostFinalizerWrapper<
+                               details::FinalizeData<T, Finalizer>::Wrapper>,
                            finalizeData,
                            &value);
   if (status != napi_ok) {
@@ -1800,7 +1819,8 @@ inline External<T> External<T>::New(napi_env env,
   napi_status status = napi_create_external(
       env,
       data,
-      details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
       finalizeData,
       &value);
   if (status != napi_ok) {
@@ -1913,7 +1933,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
       env,
       externalData,
       byteLength,
-      details::FinalizeData<void, Finalizer>::Wrapper,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<void, Finalizer>::Wrapper>,
       finalizeData,
       &value);
   if (status != napi_ok) {
@@ -1938,7 +1959,8 @@ inline ArrayBuffer ArrayBuffer::New(napi_env env,
       env,
       externalData,
       byteLength,
-      details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<void, Finalizer, Hint>::WrapperWithHint>,
       finalizeData,
       &value);
   if (status != napi_ok) {
@@ -2655,13 +2677,14 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
   details::FinalizeData<T, Finalizer>* finalizeData =
       new details::FinalizeData<T, Finalizer>(
           {std::move(finalizeCallback), nullptr});
-  napi_status status =
-      napi_create_external_buffer(env,
-                                  length * sizeof(T),
-                                  data,
-                                  details::FinalizeData<T, Finalizer>::Wrapper,
-                                  finalizeData,
-                                  &value);
+  napi_status status = napi_create_external_buffer(
+      env,
+      length * sizeof(T),
+      data,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<T, Finalizer>::Wrapper>,
+      finalizeData,
+      &value);
   if (status != napi_ok) {
     delete finalizeData;
     NAPI_THROW_IF_FAILED(env, status, Buffer());
@@ -2684,7 +2707,8 @@ inline Buffer<T> Buffer<T>::New(napi_env env,
       env,
       length * sizeof(T),
       data,
-      details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint>,
       finalizeData,
       &value);
   if (status != napi_ok) {
@@ -2723,13 +2747,14 @@ inline Buffer<T> Buffer<T>::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<T, Finalizer>::Wrapper,
-                                  finalizeData,
-                                  &value);
+  napi_status status = napi_create_external_buffer(
+      env,
+      length * sizeof(T),
+      data,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<T, Finalizer>::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<T> Buffer<T>::NewOrCopy(napi_env env,
       env,
       length * sizeof(T),
       data,
-      details::FinalizeData<T, Finalizer, Hint>::WrapperWithHint,
+      details::PostFinalizerWrapper<
+          details::FinalizeData<T, Finalizer, Hint>::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<T>::ObjectWrap(const Napi::CallbackInfo& callbackInfo) {
   napi_status status;
   napi_ref ref;
   T* instance = static_cast<T*>(this);
-  status = napi_wrap(env, wrapper, instance, FinalizeCallback, nullptr, &ref);
+  status = napi_wrap(env,
+                     wrapper,
+                     instance,
+                     details::PostFinalizerWrapper<FinalizeCallback>,
+                     nullptr,
+                     &ref);
   NAPI_THROW_IF_FAILED_VOID(env, status);
 
   Reference<Object>* instanceRef = instance;
@@ -5327,19 +5363,21 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
   auto* finalizeData = new details::
       ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
           {data, finalizeCallback});
-  napi_status status = napi_create_threadsafe_function(
-      env,
-      nullptr,
-      nullptr,
-      String::From(env, resourceName),
-      maxQueueSize,
-      initialThreadCount,
-      finalizeData,
+  auto fini =
       details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
-          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<ContextType, DataType, CallJs>::New(
   auto* finalizeData = new details::
       ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
           {data, finalizeCallback});
-  napi_status status = napi_create_threadsafe_function(
-      env,
-      nullptr,
-      resource,
-      String::From(env, resourceName),
-      maxQueueSize,
-      initialThreadCount,
-      finalizeData,
+  auto fini =
       details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
-          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<ContextType, DataType, CallJs>::New(
   auto* finalizeData = new details::
       ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
           {data, finalizeCallback});
-  napi_status status = napi_create_threadsafe_function(
-      env,
-      callback,
-      nullptr,
-      String::From(env, resourceName),
-      maxQueueSize,
-      initialThreadCount,
-      finalizeData,
+  auto fini =
       details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
-          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<ContextType, DataType, CallJs>::New(
   auto* finalizeData = new details::
       ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>(
           {data, finalizeCallback});
+  auto fini =
+      details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
+          FinalizeFinalizeWrapperWithDataAndContext;
   napi_status status = napi_create_threadsafe_function(
       env,
       details::DefaultCallbackWrapper<
@@ -5544,8 +5589,7 @@ TypedThreadSafeFunction<ContextType, DataType, CallJs>::New(
       maxQueueSize,
       initialThreadCount,
       finalizeData,
-      details::ThreadSafeFinalize<ContextType, Finalizer, FinalizerDataType>::
-          FinalizeFinalizeWrapperWithDataAndContext,
+      fini,
       context,
       CallJsInternal,
       &tsfn._tsfn);

diff --git 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']
       }],