fix(napi): race issues with Node.js worker_thread (#1081)

Co-authored-by: Jan Piotrowski <piotrowski+github@gmail.com>
napi-rs · Mar 5, 2022 · 9f3fbaa · 9f3fbaa
1 parent 1bee150
commit 9f3fbaa
Show file tree

Hide file tree

Showing 28 changed files with 292 additions and 246 deletions.
diff --git a/.cirrus.yml b/.cirrus.yml
@@ -1,5 +1,5 @@
 freebsd_instance:
-  image: freebsd-12-2-release-amd64
+  image: freebsd-13-0-release-amd64
 
 task:
   name: FreeBSD
@@ -8,7 +8,8 @@ task:
     RUSTUP_HOME: /usr/local/rustup
     CARGO_HOME: /usr/local/cargo
     PATH: /usr/local/cargo/bin:$PATH
-    RUSTUP_IO_THREADS: 1
+    RUSTUP_IO_THREADS: '1'
+    CI: '1'
   setup_script:
     - pkg update
     - pkg install -y -f curl node16 libnghttp2
@@ -28,4 +29,4 @@ task:
     - yarn build
     - cargo test -p napi-sys --lib -- --nocapture
     - yarn build:test
-    - yarn test
+    - yarn test --verbose
diff --git a/.github/workflows/android-armv7.yml b/.github/workflows/android-armv7.yml
@@ -17,7 +17,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           cache: 'yarn'

diff --git a/.github/workflows/android.yml b/.github/workflows/android.yml
@@ -17,7 +17,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           cache: 'yarn'

diff --git a/.github/workflows/asan.yml b/.github/workflows/asan.yml
@@ -24,7 +24,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: ${{ matrix.node }}
           check-latest: true

diff --git a/.github/workflows/bench.yaml b/.github/workflows/bench.yaml
@@ -19,7 +19,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml
@@ -15,7 +15,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/linux-aarch64-musl.yaml b/.github/workflows/linux-aarch64-musl.yaml
@@ -20,7 +20,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/linux-aarch64.yaml b/.github/workflows/linux-aarch64.yaml
@@ -21,7 +21,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/linux-armv7.yaml b/.github/workflows/linux-armv7.yaml
@@ -21,7 +21,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/linux-musl.yaml b/.github/workflows/linux-musl.yaml
@@ -18,7 +18,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/memory-test.yml b/.github/workflows/memory-test.yml
@@ -18,7 +18,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml
@@ -24,7 +24,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: ${{ matrix.node }}
           check-latest: true
@@ -46,16 +46,16 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ~/.cargo/registry
-          key: stable-${{ matrix.os }}-node@${{ matrix.node }}-cargo-registry-trimmed
+          key: stable-${{ matrix.os }}-node@${{ matrix.node }}-cargo-registry
 
       - name: Cache cargo index
         uses: actions/cache@v2
         with:
           path: ~/.cargo/git
-          key: stable-${{ matrix.os }}gnu-node@${{ matrix.node }}-cargo-index-trimmed
+          key: stable-${{ matrix.os }}-node@${{ matrix.node }}-cargo-index
 
       - name: 'Install dependencies'
-        run: yarn install --immutable --network-timeout 300000
+        run: yarn install --mode=skip-build --immutable --network-timeout 300000
 
       - name: 'Build TypeScript'
         run: yarn build
@@ -69,7 +69,7 @@ jobs:
       - name: Unit tests
         run: |
           yarn build:test
-          yarn test
+          yarn test --verbose
         env:
           RUST_BACKTRACE: 1
 

diff --git a/.github/workflows/windows-arm.yml b/.github/workflows/windows-arm.yml
@@ -18,14 +18,14 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true
           cache: 'yarn'
 
       - name: 'Install dependencies'
-        run: yarn install --immutable --network-timeout 300000
+        run: yarn install --mode=skip-build --immutable --network-timeout 300000
 
       - name: 'Build TypeScript'
         run: yarn build

diff --git a/.github/workflows/windows-i686.yml b/.github/workflows/windows-i686.yml
@@ -18,15 +18,15 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true
           architecture: 'x86'
           cache: 'yarn'
 
       - name: 'Install dependencies'
-        run: yarn install --immutable --network-timeout 300000
+        run: yarn install --mode=skip-build --immutable --network-timeout 300000
 
       - name: 'Build TypeScript'
         run: yarn build
@@ -50,13 +50,13 @@ jobs:
         uses: actions/cache@v2
         with:
           path: ~/.cargo/registry
-          key: stable-windows-i686-node@lts-cargo-registry-trimmed-${{ hashFiles('**/Cargo.lock') }}
+          key: stable-windows-i686-node@lts-cargo-registry
 
       - name: Cache cargo index
         uses: actions/cache@v2
         with:
           path: ~/.cargo/git
-          key: stable-windows-i686-node@lts-cargo-index-trimmed-${{ hashFiles('**/Cargo.lock') }}
+          key: stable-windows-i686-node@lts-cargo-index
 
       - name: Check build
         uses: actions-rs/cargo@v1
@@ -66,9 +66,9 @@ jobs:
 
       - name: Build Tests
         run: |
-          yarn --cwd ./examples/napi-compat-mode build-i686
-          yarn --cwd ./examples/napi build-i686
-          yarn test
+          yarn --cwd ./examples/napi-compat-mode build-i686 --release
+          yarn --cwd ./examples/napi build-i686 --release
+          yarn test --verbose
         env:
           RUST_BACKTRACE: 1
 

diff --git a/.github/workflows/zig.yaml b/.github/workflows/zig.yaml
@@ -27,7 +27,7 @@ jobs:
       - run: docker run --rm --privileged multiarch/qemu-user-static:register --reset
       - uses: actions/checkout@v2
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           # Testing for compatibility with node v12.x
           node-version: 12
@@ -93,7 +93,7 @@ jobs:
         if: matrix.settings.host == 'ubuntu-latest'
       - uses: actions/checkout@v2
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/ava.config.mjs b/ava.config.mjs
@@ -1,3 +1,5 @@
+import { cpus } from 'os'
+
 const configuration = {
   extensions: ['ts', 'tsx'],
   files: ['cli/**/*.spec.ts', 'examples/**/__test__/**/*.spec.ts'],
@@ -6,7 +8,10 @@ const configuration = {
     TS_NODE_PROJECT: './examples/tsconfig.json',
   },
   timeout: '1m',
-  workerThreads: false,
+  workerThreads: true,
+  concurrency: process.env.CI ? 2 : cpus().length,
+  failFast: false,
+  verbose: !!process.env.CI,
 }
 
 if (parseInt(process.versions.napi, 10) < 4) {

diff --git a/cli/src/new/ci-template.ts b/cli/src/new/ci-template.ts
@@ -123,7 +123,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true
@@ -255,7 +255,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: \${{ matrix.node }}
           check-latest: true
@@ -297,7 +297,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: \${{ matrix.node }}
           check-latest: true
@@ -339,7 +339,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: \${{ matrix.node }}
           check-latest: true
@@ -512,7 +512,7 @@ jobs:
       - uses: actions/checkout@v2
 
       - name: Setup node
-        uses: actions/setup-node@v2
+        uses: actions/setup-node@v3
         with:
           node-version: 16
           check-latest: true

diff --git a/crates/backend/src/codegen/fn.rs b/crates/backend/src/codegen/fn.rs
@@ -49,7 +49,7 @@ impl TryToTokens for NapiFn {
       quote! {
         // constructor function is called from class `factory`
         // so we should skip the original `constructor` logic
-        if napi::bindgen_prelude::___CALL_FROM_FACTORY.load(std::sync::atomic::Ordering::Relaxed) {
+        if napi::bindgen_prelude::___CALL_FROM_FACTORY.with(|inner| inner.load(std::sync::atomic::Ordering::Relaxed)) {
           return std::ptr::null_mut();
         }
         napi::bindgen_prelude::CallbackInfo::<#args_len>::new(env, cb, None).and_then(|mut cb| {
@@ -319,7 +319,7 @@ impl NapiFn {
             "Failed to register function `{}`",
             #name_str,
           )?;
-          napi::bindgen_prelude::register_js_function(#js_name, env, #cb_name, Some(#intermediate_ident));
+          napi::bindgen_prelude::register_js_function(#js_name, #cb_name, Some(#intermediate_ident));
           Ok(fn_ptr)
         }
 

diff --git a/crates/backend/src/codegen/struct.rs b/crates/backend/src/codegen/struct.rs
@@ -232,7 +232,7 @@ impl NapiStruct {
             )?;
 
             let mut result = std::ptr::null_mut();
-            napi::bindgen_prelude::___CALL_FROM_FACTORY.store(true, std::sync::atomic::Ordering::Relaxed);
+            napi::bindgen_prelude::___CALL_FROM_FACTORY.with(|inner| inner.store(true, std::sync::atomic::Ordering::Relaxed));
             napi::check_status!(
               napi::sys::napi_new_instance(env, ctor, 0, std::ptr::null_mut(), &mut result),
               "Failed to construct class `{}`",
@@ -250,7 +250,7 @@ impl NapiStruct {
               "Failed to wrap native object of class `{}`",
               #js_name_str
             )?;
-            napi::bindgen_prelude::___CALL_FROM_FACTORY.store(false, std::sync::atomic::Ordering::Relaxed);
+            napi::bindgen_prelude::___CALL_FROM_FACTORY.with(|inner| inner.store(false, std::sync::atomic::Ordering::Relaxed));
             Ok(result)
           } else {
             Err(napi::bindgen_prelude::Error::new(

diff --git a/crates/napi/src/bindgen_runtime/callback_info.rs b/crates/napi/src/bindgen_runtime/callback_info.rs
@@ -4,11 +4,11 @@ use std::sync::atomic::{AtomicBool, Ordering};
 
 use crate::{bindgen_prelude::*, check_status, sys, Result};
 
-#[doc(hidden)]
-/// Determined is `constructor` called from Class `factory`
-/// Ugly but works
-/// We can even be more ugly without `atomic`
-pub static ___CALL_FROM_FACTORY: AtomicBool = AtomicBool::new(false);
+thread_local! {
+  #[doc(hidden)]
+  /// Determined is `constructor` called from Class `factory`
+  pub static ___CALL_FROM_FACTORY: AtomicBool = AtomicBool::new(false);
+}
 
 pub struct CallbackInfo<const N: usize> {
   env: sys::napi_env,
@@ -90,9 +90,9 @@ impl<const N: usize> CallbackInfo<N> {
     let this = self.this();
     let mut instance = ptr::null_mut();
     unsafe {
-      ___CALL_FROM_FACTORY.store(true, Ordering::Relaxed);
+      ___CALL_FROM_FACTORY.with(|inner| inner.store(true, Ordering::Relaxed));
       let status = sys::napi_new_instance(self.env, this, 0, ptr::null_mut(), &mut instance);
-      ___CALL_FROM_FACTORY.store(false, Ordering::Relaxed);
+      ___CALL_FROM_FACTORY.with(|inner| inner.store(false, Ordering::Relaxed));
       // Error thrown in `constructor`
       if status == sys::Status::napi_pending_exception {
         let mut exception = ptr::null_mut();

diff --git a/crates/napi/src/bindgen_runtime/js_values/number.rs b/crates/napi/src/bindgen_runtime/js_values/number.rs
@@ -1,4 +1,5 @@
 use super::{check_status, sys, Result};
+use crate::type_of;
 
 macro_rules! impl_number_conversions {
 	( $( ($name:literal, $t:ty, $get:ident, $create:ident) ,)* ) => {
@@ -43,11 +44,12 @@ macro_rules! impl_number_conversions {
 
           check_status!(
             unsafe { sys::$get(env, napi_val, &mut ret) },
-						"Failed to convert napi value into rust type `{}`",
-            $name
+            "Failed to convert napi value {:?} into rust type `{}`",
+            type_of!(env, napi_val),
+            $name,
           )?;
 
-					Ok(ret)
+            Ok(ret)
 				}
       }
 		)*