From 9f3fbaa8e0b6c0bcdd740d39d16de35a4ec18aa8 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Sat, 5 Mar 2022 14:14:32 +0800 Subject: [PATCH] fix(napi): race issues with Node.js worker_thread (#1081) Co-authored-by: Jan Piotrowski --- .cirrus.yml | 7 +- .github/workflows/android-armv7.yml | 2 +- .github/workflows/android.yml | 2 +- .github/workflows/asan.yml | 2 +- .github/workflows/bench.yaml | 2 +- .github/workflows/lint.yaml | 2 +- .github/workflows/linux-aarch64-musl.yaml | 2 +- .github/workflows/linux-aarch64.yaml | 2 +- .github/workflows/linux-armv7.yaml | 2 +- .github/workflows/linux-musl.yaml | 2 +- .github/workflows/memory-test.yml | 2 +- .github/workflows/test.yaml | 10 +- .github/workflows/windows-arm.yml | 4 +- .github/workflows/windows-i686.yml | 14 +- .github/workflows/zig.yaml | 4 +- ava.config.mjs | 7 +- cli/src/new/ci-template.ts | 10 +- crates/backend/src/codegen/fn.rs | 4 +- crates/backend/src/codegen/struct.rs | 4 +- .../napi/src/bindgen_runtime/callback_info.rs | 14 +- .../src/bindgen_runtime/js_values/number.rs | 8 +- .../src/bindgen_runtime/module_register.rs | 385 ++++++++++-------- crates/napi/src/js_values/de.rs | 6 +- crates/napi/src/js_values/mod.rs | 2 +- crates/napi/src/tokio_runtime.rs | 2 +- examples/napi/__test__/worker-thread.spec.ts | 33 +- examples/napi/src/callback.rs | 4 +- package.json | 4 +- 28 files changed, 294 insertions(+), 248 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index 82b6649a..073a0353 100644 --- 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 index 26deee03..2c427ae3 100644 --- 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 index f3e772e6..4c6eea92 100644 --- 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 index 8acfcdf5..46b9a40a 100644 --- 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 index 2fa975c1..840d2ba5 100644 --- 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 index cefb92b7..548216a0 100644 --- 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 index 95cd1ba1..ec962a2b 100644 --- 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 index 04397721..0dce5cc1 100644 --- 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 index 01ee089e..ec720cf8 100644 --- 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 index 4b4e374c..184543e3 100644 --- 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 index 83a86f79..fc96e010 100644 --- 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 index 4b41246b..c4740091 100644 --- 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 index 819b4193..56753909 100644 --- 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 index 403724ff..044e38a2 100644 --- a/.github/workflows/windows-i686.yml +++ b/.github/workflows/windows-i686.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 @@ -26,7 +26,7 @@ jobs: 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 index be1bee5c..92b55a45 100644 --- 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 index 42906ac1..8d50ff93 100644 --- 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 index 33920707..cc1d6e6e 100644 --- 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 index c0fff9c8..1a58b472 100644 --- 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 index a3c431c4..dcabd542 100644 --- 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 index d52cbd5c..50409046 100644 --- 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 { env: sys::napi_env, @@ -90,9 +90,9 @@ impl CallbackInfo { 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 index c44cf9e8..36e7c6c0 100644 --- 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) } } )* diff --git a/crates/napi/src/bindgen_runtime/module_register.rs b/crates/napi/src/bindgen_runtime/module_register.rs index ea4849a6..740e7000 100644 --- a/crates/napi/src/bindgen_runtime/module_register.rs +++ b/crates/napi/src/bindgen_runtime/module_register.rs @@ -1,14 +1,18 @@ use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; +#[cfg(all(feature = "tokio_rt", feature = "napi4"))] use std::ffi::c_void; -use std::ffi::CStr; +use std::ffi::{CStr, CString}; +use std::mem; use std::ptr; -use std::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; +use std::sync::atomic::AtomicBool; +use std::sync::{atomic::Ordering, Mutex}; use lazy_static::lazy_static; use crate::{ - check_status, check_status_or_throw, sys, JsError, JsFunction, Property, Result, Value, ValueType, + check_status, check_status_or_throw, sys, Env, JsError, JsFunction, Property, Result, Value, + ValueType, }; pub type ExportRegisterCallback = unsafe fn(sys::napi_env) -> Result; @@ -16,64 +20,61 @@ pub type ModuleExportsCallback = unsafe fn(env: sys::napi_env, exports: sys::napi_value) -> Result<()>; struct PersistedSingleThreadVec { - inner: AtomicPtr, - length: AtomicUsize, + inner: Mutex>, } impl Default for PersistedSingleThreadVec { fn default() -> Self { - let mut vec: Vec = Vec::with_capacity(1); - let ret = PersistedSingleThreadVec { - inner: AtomicPtr::new(vec.as_mut_ptr()), - length: AtomicUsize::new(0), - }; - std::mem::forget(vec); - ret + PersistedSingleThreadVec { + inner: Mutex::new(Vec::new()), + } } } impl PersistedSingleThreadVec { #[allow(clippy::mut_from_ref)] - fn borrow_mut(&self) -> &mut [T] { - let length = self.length.load(Ordering::Relaxed); - if length == 0 { - return &mut []; - } - let inner = self.inner.load(Ordering::Relaxed); - unsafe { std::slice::from_raw_parts_mut(inner, length) } + fn borrow_mut(&self, f: F) + where + F: FnOnce(&mut [T]), + { + let mut locked = self + .inner + .lock() + .expect("Acquire persisted thread vec lock failed"); + f(&mut *locked); } fn push(&self, item: T) { - let length = self.length.load(Ordering::Relaxed); - let inner = self.inner.load(Ordering::Relaxed); - let mut temp = unsafe { Vec::from_raw_parts(inner, length, length) }; - temp.push(item); - // Inner Vec has been reallocated, so we need to update the pointer - if temp.as_mut_ptr() != inner { - self.inner.store(temp.as_mut_ptr(), Ordering::Relaxed); - } - std::mem::forget(temp); - - self.length.fetch_add(1, Ordering::Relaxed); + let mut locked = self + .inner + .lock() + .expect("Acquire persisted thread vec lock failed"); + locked.push(item); } } unsafe impl Send for PersistedSingleThreadVec {} unsafe impl Sync for PersistedSingleThreadVec {} -struct PersistedSingleThreadHashMap(*mut HashMap); +struct PersistedSingleThreadHashMap(Mutex>); impl PersistedSingleThreadHashMap { #[allow(clippy::mut_from_ref)] - fn borrow_mut(&self) -> &mut HashMap { - unsafe { Box::leak(Box::from_raw(self.0)) } + fn borrow_mut(&self, f: F) -> R + where + F: FnOnce(&mut HashMap) -> R, + { + let mut lock = self + .0 + .lock() + .expect("Acquire persisted thread hash map lock failed"); + f(&mut *lock) } } impl Default for PersistedSingleThreadHashMap { fn default() -> Self { - let map = Default::default(); - PersistedSingleThreadHashMap(Box::into_raw(Box::new(map))) + PersistedSingleThreadHashMap(Mutex::new(Default::default())) } } @@ -85,18 +86,21 @@ type ModuleClassProperty = PersistedSingleThreadHashMap< HashMap, (&'static str, Vec)>, >; -type FnRegisterMap = PersistedSingleThreadHashMap< - ExportRegisterCallback, - (sys::napi_env, sys::napi_callback, &'static str), ->; - unsafe impl Send for PersistedSingleThreadHashMap {} unsafe impl Sync for PersistedSingleThreadHashMap {} lazy_static! { static ref MODULE_REGISTER_CALLBACK: ModuleRegisterCallback = Default::default(); static ref MODULE_CLASS_PROPERTIES: ModuleClassProperty = Default::default(); - static ref FN_REGISTER_MAP: FnRegisterMap = Default::default(); + static ref MODULE_REGISTER_LOCK: Mutex<()> = Mutex::new(()); + static ref REGISTERED: AtomicBool = AtomicBool::new(false); +} + +#[inline] +fn wait_first_thread_registered() { + while !REGISTERED.load(Ordering::SeqCst) { + std::hint::spin_loop(); + } } #[cfg(feature = "compat-mode")] @@ -107,13 +111,15 @@ lazy_static! { thread_local! { static REGISTERED_CLASSES: RefCell> = Default::default(); + static FN_REGISTER_MAP: RefCell> = Default::default(); } #[doc(hidden)] pub fn get_class_constructor(js_name: &'static str) -> Option { + wait_first_thread_registered(); REGISTERED_CLASSES.with(|registered_classes| { let classes = registered_classes.borrow(); classes.get(js_name).copied() @@ -139,11 +145,12 @@ pub fn register_module_export( #[doc(hidden)] pub fn register_js_function( name: &'static str, - env: sys::napi_env, cb: ExportRegisterCallback, c_fn: sys::napi_callback, ) { - FN_REGISTER_MAP.borrow_mut().insert(cb, (env, c_fn, name)); + FN_REGISTER_MAP.with(|inner| { + inner.borrow_mut().insert(cb, (c_fn, name.to_owned())); + }); } #[doc(hidden)] @@ -153,12 +160,12 @@ pub fn register_class( js_name: &'static str, props: Vec, ) { - let map = MODULE_CLASS_PROPERTIES.borrow_mut(); - let val = map.entry(rust_name).or_default(); - let val = val.entry(js_mod).or_default(); - - val.0 = js_name; - val.1.extend(props.into_iter()); + MODULE_CLASS_PROPERTIES.borrow_mut(|inner| { + let val = inner.entry(rust_name).or_default(); + let val = val.entry(js_mod).or_default(); + val.0 = js_name; + val.1.extend(props.into_iter()); + }); } #[inline] @@ -179,37 +186,40 @@ pub fn register_class( /// returnSomeFn()(); // 1 /// ``` /// -pub fn get_js_function(raw_fn: ExportRegisterCallback) -> Result { - FN_REGISTER_MAP - .borrow_mut() - .get(&raw_fn) - .and_then(|(env, cb, name)| { - let mut function = ptr::null_mut(); - let name_len = name.len() - 1; - let fn_name = unsafe { CStr::from_bytes_with_nul_unchecked(name.as_bytes()) }; - check_status!(unsafe { - sys::napi_create_function( - *env, - fn_name.as_ptr(), - name_len, - *cb, - ptr::null_mut(), - &mut function, +pub fn get_js_function(env: &Env, raw_fn: ExportRegisterCallback) -> Result { + wait_first_thread_registered(); + FN_REGISTER_MAP.with(|inner| { + inner + .borrow() + .get(&raw_fn) + .and_then(|(cb, name)| { + let mut function = ptr::null_mut(); + let name_len = name.len() - 1; + let fn_name = unsafe { CStr::from_bytes_with_nul_unchecked(name.as_bytes()) }; + check_status!(unsafe { + sys::napi_create_function( + env.0, + fn_name.as_ptr(), + name_len, + *cb, + ptr::null_mut(), + &mut function, + ) + }) + .ok()?; + Some(JsFunction(Value { + env: env.0, + value: function, + value_type: ValueType::Function, + })) + }) + .ok_or_else(|| { + crate::Error::new( + crate::Status::InvalidArg, + "JavaScript function does not exist".to_owned(), ) }) - .ok()?; - Some(JsFunction(Value { - env: *env, - value: function, - value_type: ValueType::Function, - })) - }) - .ok_or_else(|| { - crate::Error::new( - crate::Status::InvalidArg, - "JavaScript function does not exist".to_owned(), - ) - }) + }) } /// Get `C Callback` from defined Rust `fn` @@ -232,16 +242,19 @@ pub fn get_js_function(raw_fn: ExportRegisterCallback) -> Result { /// ``` /// pub fn get_c_callback(raw_fn: ExportRegisterCallback) -> Result { - FN_REGISTER_MAP - .borrow_mut() - .get(&raw_fn) - .and_then(|(_env, cb, _name)| *cb) - .ok_or_else(|| { - crate::Error::new( - crate::Status::InvalidArg, - "JavaScript function does not exist".to_owned(), - ) - }) + wait_first_thread_registered(); + FN_REGISTER_MAP.with(|inner| { + inner + .borrow() + .get(&raw_fn) + .and_then(|(cb, _name)| *cb) + .ok_or_else(|| { + crate::Error::new( + crate::Status::InvalidArg, + "JavaScript function does not exist".to_owned(), + ) + }) + }) } #[no_mangle] @@ -249,81 +262,103 @@ unsafe extern "C" fn napi_register_module_v1( env: sys::napi_env, exports: sys::napi_value, ) -> sys::napi_value { - let mut exports_objects: HashMap, sys::napi_value> = HashMap::default(); - MODULE_REGISTER_CALLBACK - .borrow_mut() - .iter_mut() - .fold( - HashMap::, Vec<(&'static str, ExportRegisterCallback)>>::new(), - |mut acc, (js_mod, item)| { - if let Some(k) = acc.get_mut(js_mod) { - k.push(*item); - } else { - acc.insert(*js_mod, vec![*item]); - } - acc - }, - ) - .iter() - .for_each(|(js_mod, items)| { - let mut exports_js_mod = ptr::null_mut(); - if let Some(js_mod_str) = js_mod { - if let Some(exports_object) = exports_objects.get(js_mod) { - exports_js_mod = *exports_object; - } else { - check_status_or_throw!( - env, - unsafe { sys::napi_create_object(env, &mut exports_js_mod) }, - "Create export JavaScript Object [{}] failed", - js_mod_str - ); - check_status_or_throw!( - env, - unsafe { - sys::napi_set_named_property( - env, - exports, - js_mod_str.as_ptr() as *const _, - exports_js_mod, - ) - }, - "Set exports Object [{}] into exports object failed", - js_mod_str - ); - exports_objects.insert(*js_mod, exports_js_mod); - } - } - for (name, callback) in items { - let js_name = unsafe { CStr::from_bytes_with_nul_unchecked(name.as_bytes()) }; - unsafe { - if let Err(e) = callback(env).and_then(|v| { - let exported_object = if exports_js_mod.is_null() { - exports - } else { - exports_js_mod - }; - check_status!( - sys::napi_set_named_property(env, exported_object, js_name.as_ptr(), v), - "Failed to register export `{}`", - name, - ) - }) { - JsError::from(e).throw_into(env) + let lock = MODULE_REGISTER_LOCK + .lock() + .expect("Failed to acquire module register lock"); + let mut exports_objects: HashSet = HashSet::default(); + MODULE_REGISTER_CALLBACK.borrow_mut(|inner| { + inner + .iter_mut() + .fold( + HashMap::, Vec<(&'static str, ExportRegisterCallback)>>::new(), + |mut acc, (js_mod, item)| { + if let Some(k) = acc.get_mut(js_mod) { + k.push(*item); + } else { + acc.insert(*js_mod, vec![*item]); + } + acc + }, + ) + .iter() + .for_each(|(js_mod, items)| { + let mut exports_js_mod = ptr::null_mut(); + if let Some(js_mod_str) = js_mod { + let mod_name_c_str = + unsafe { CStr::from_bytes_with_nul_unchecked(js_mod_str.as_bytes()) }; + if exports_objects.contains(*js_mod_str) { + check_status_or_throw!( + env, + unsafe { + sys::napi_get_named_property( + env, + exports, + mod_name_c_str.as_ptr(), + &mut exports_js_mod, + ) + }, + "Get mod {} from exports failed", + js_mod_str, + ); + } else { + check_status_or_throw!( + env, + unsafe { sys::napi_create_object(env, &mut exports_js_mod) }, + "Create export JavaScript Object [{}] failed", + js_mod_str + ); + check_status_or_throw!( + env, + unsafe { + sys::napi_set_named_property(env, exports, mod_name_c_str.as_ptr(), exports_js_mod) + }, + "Set exports Object [{}] into exports object failed", + js_mod_str + ); + exports_objects.insert(js_mod_str.to_string()); } } - } - }); + for (name, callback) in items { + unsafe { + let js_name = CStr::from_bytes_with_nul_unchecked(name.as_bytes()); + if let Err(e) = callback(env).and_then(|v| { + let exported_object = if exports_js_mod.is_null() { + exports + } else { + exports_js_mod + }; + check_status!( + sys::napi_set_named_property(env, exported_object, js_name.as_ptr(), v), + "Failed to register export `{}`", + name, + ) + }) { + JsError::from(e).throw_into(env) + } + } + } + }) + }); - MODULE_CLASS_PROPERTIES - .borrow_mut() - .iter() - .for_each(|(rust_name, js_mods)| { + MODULE_CLASS_PROPERTIES.borrow_mut(|inner| { + inner.iter().for_each(|(rust_name, js_mods)| { for (js_mod, (js_name, props)) in js_mods { let mut exports_js_mod = ptr::null_mut(); unsafe { if let Some(js_mod_str) = js_mod { - if let Some(exports_object) = exports_objects.get(js_mod) { - exports_js_mod = *exports_object; + let mod_name_c_str = CStr::from_bytes_with_nul_unchecked(js_mod_str.as_bytes()); + if exports_objects.contains(*js_mod_str) { + check_status_or_throw!( + env, + sys::napi_get_named_property( + env, + exports, + mod_name_c_str.as_ptr(), + &mut exports_js_mod, + ), + "Get mod {} from exports failed", + js_mod_str, + ); } else { check_status_or_throw!( env, @@ -333,16 +368,11 @@ unsafe extern "C" fn napi_register_module_v1( ); check_status_or_throw!( env, - sys::napi_set_named_property( - env, - exports, - js_mod_str.as_ptr() as *const _, - exports_js_mod - ), + sys::napi_set_named_property(env, exports, mod_name_c_str.as_ptr(), exports_js_mod), "Set exports Object [{}] into exports object failed", js_mod_str ); - exports_objects.insert(*js_mod, exports_js_mod); + exports_objects.insert(js_mod_str.to_string()); } } let (ctor, props): (Vec<_>, Vec<_>) = props.iter().partition(|prop| prop.is_ctor); @@ -354,7 +384,7 @@ unsafe extern "C" fn napi_register_module_v1( let ctor = ctor.get(0).map(|c| c.raw().method.unwrap()).unwrap_or(noop); let raw_props: Vec<_> = props.iter().map(|prop| prop.raw()).collect(); - let js_class_name = CStr::from_bytes_with_nul_unchecked(js_name.as_bytes()); + let js_class_name = CString::from_vec_with_nul_unchecked(js_name.as_bytes().to_vec()); let mut class_ptr = ptr::null_mut(); check_status_or_throw!( @@ -362,7 +392,7 @@ unsafe extern "C" fn napi_register_module_v1( sys::napi_define_class( env, js_class_name.as_ptr(), - js_name.len(), + js_name.len() - 1, Some(ctor), ptr::null_mut(), raw_props.len(), @@ -379,7 +409,7 @@ unsafe extern "C" fn napi_register_module_v1( REGISTERED_CLASSES.with(|registered_classes| { let mut registered_class = registered_classes.borrow_mut(); - registered_class.insert(js_name, ctor_ref); + registered_class.insert(js_name.to_string(), ctor_ref); }); check_status_or_throw!( @@ -400,22 +430,22 @@ unsafe extern "C" fn napi_register_module_v1( ); } } - }); + }) + }); #[cfg(feature = "compat-mode")] - MODULE_EXPORTS - .borrow_mut() - .iter() - .for_each(|callback| unsafe { + MODULE_EXPORTS.borrow_mut(|inner| { + inner.iter().for_each(|callback| unsafe { if let Err(e) = callback(env, exports) { JsError::from(e).throw_into(env); } - }); + }) + }); #[cfg(all(feature = "tokio_rt", feature = "napi4"))] { let _ = crate::tokio_runtime::RT.clone(); - crate::tokio_runtime::TOKIO_RT_REF_COUNT.fetch_add(1, Ordering::Relaxed); + crate::tokio_runtime::TOKIO_RT_REF_COUNT.fetch_add(1, Ordering::SeqCst); assert_eq!( unsafe { sys::napi_add_env_cleanup_hook(env, Some(crate::shutdown_tokio_rt), env as *mut c_void) @@ -423,7 +453,8 @@ unsafe extern "C" fn napi_register_module_v1( sys::Status::napi_ok ); } - + mem::drop(lock); + REGISTERED.store(true, Ordering::SeqCst); exports } @@ -431,7 +462,7 @@ pub(crate) unsafe extern "C" fn noop( env: sys::napi_env, _info: sys::napi_callback_info, ) -> sys::napi_value { - if !crate::bindgen_runtime::___CALL_FROM_FACTORY.load(std::sync::atomic::Ordering::Relaxed) { + if !crate::bindgen_runtime::___CALL_FROM_FACTORY.with(|inner| inner.load(Ordering::Relaxed)) { unsafe { sys::napi_throw_error( env, diff --git a/crates/napi/src/js_values/de.rs b/crates/napi/src/js_values/de.rs index 425f6b52..b0b69057 100644 --- a/crates/napi/src/js_values/de.rs +++ b/crates/napi/src/js_values/de.rs @@ -20,7 +20,7 @@ impl<'x, 'de, 'env> serde::de::Deserializer<'x> for &'de mut De<'env> { where V: Visitor<'x>, { - let js_value_type = unsafe { type_of!(self.0.env, self.0.value) }?; + let js_value_type = type_of!(self.0.env, self.0.value)?; match js_value_type { ValueType::Null | ValueType::Undefined => visitor.visit_unit(), ValueType::Boolean => { @@ -89,7 +89,7 @@ impl<'x, 'de, 'env> serde::de::Deserializer<'x> for &'de mut De<'env> { where V: Visitor<'x>, { - match unsafe { type_of!(self.0.env, self.0.value) }? { + match type_of!(self.0.env, self.0.value)? { ValueType::Undefined | ValueType::Null => visitor.visit_none(), _ => visitor.visit_some(self), } @@ -104,7 +104,7 @@ impl<'x, 'de, 'env> serde::de::Deserializer<'x> for &'de mut De<'env> { where V: Visitor<'x>, { - let js_value_type = unsafe { type_of!(self.0.env, self.0.value)? }; + let js_value_type = type_of!(self.0.env, self.0.value)?; match js_value_type { ValueType::String => visitor.visit_enum(JsEnumAccess::new( unsafe { JsString::from_raw_unchecked(self.0.env, self.0.value) } diff --git a/crates/napi/src/js_values/mod.rs b/crates/napi/src/js_values/mod.rs index 4ee05cd5..0cb7ae21 100644 --- a/crates/napi/src/js_values/mod.rs +++ b/crates/napi/src/js_values/mod.rs @@ -669,7 +669,7 @@ impl<'env> NapiRaw for &'env JsUnknown { impl JsUnknown { pub fn get_type(&self) -> Result { - unsafe { type_of!(self.0.env, self.0.value) } + type_of!(self.0.env, self.0.value) } /// # Safety diff --git a/crates/napi/src/tokio_runtime.rs b/crates/napi/src/tokio_runtime.rs index 9fbac24b..133d66e0 100644 --- a/crates/napi/src/tokio_runtime.rs +++ b/crates/napi/src/tokio_runtime.rs @@ -36,7 +36,7 @@ pub(crate) static TOKIO_RT_REF_COUNT: AtomicUsize = AtomicUsize::new(0); #[doc(hidden)] #[inline(never)] pub unsafe extern "C" fn shutdown_tokio_rt(arg: *mut c_void) { - if TOKIO_RT_REF_COUNT.fetch_sub(1, Ordering::Relaxed) == 0 { + if TOKIO_RT_REF_COUNT.fetch_sub(1, Ordering::SeqCst) == 0 { let sender = &RT.1; if let Err(e) = sender.clone().try_send(()) { match e { diff --git a/examples/napi/__test__/worker-thread.spec.ts b/examples/napi/__test__/worker-thread.spec.ts index 337fb5e8..9d1c7804 100644 --- a/examples/napi/__test__/worker-thread.spec.ts +++ b/examples/napi/__test__/worker-thread.spec.ts @@ -3,18 +3,25 @@ import { Worker } from 'worker_threads' import test from 'ava' -import { DEFAULT_COST, Animal, Kind } from '../index' +import { Animal, Kind, DEFAULT_COST } from '../index' -test('should be able to require in worker thread', (t) => { - const w = new Worker(join(__dirname, 'worker.js')) - return new Promise((resolve) => { - w.on('message', (msg) => { - t.is(msg, Animal.withKind(Kind.Cat).whoami() + DEFAULT_COST) - resolve() - }) - }) - .then(() => w.terminate()) - .then(() => { - t.pass() - }) +test('should be able to require in worker thread', async (t) => { + await Promise.all( + Array.from({ length: 100 }).map(() => { + const w = new Worker(join(__dirname, 'worker.js')) + return new Promise((resolve, reject) => { + w.on('message', (msg) => { + t.is(msg, Animal.withKind(Kind.Cat).whoami() + DEFAULT_COST) + resolve() + }) + w.on('error', (err) => { + reject(err) + }) + }) + .then(() => w.terminate()) + .then(() => { + t.pass() + }) + }), + ) }) diff --git a/examples/napi/src/callback.rs b/examples/napi/src/callback.rs index eaa1da9b..52f1269b 100644 --- a/examples/napi/src/callback.rs +++ b/examples/napi/src/callback.rs @@ -43,6 +43,6 @@ fn read_file_content() -> Result { } #[napi] -fn return_js_function() -> Result { - get_js_function(read_file_js_function) +fn return_js_function(env: Env) -> Result { + get_js_function(&env, read_file_js_function) } diff --git a/package.json b/package.json index 33eef455..ce512e90 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,7 @@ "format:toml": "taplo format", "lint": "eslint -c .eslintrc.yml .", "prepublishOnly": "npm run build && pinst --disable", - "test": "ava", + "test": "ava \"./examples/napi/**/*.ts\" && ava --no-worker-threads \"./examples/napi-compat-mode/**/*.ts\" && ava \"./cli/**/*.ts\"", "test:memory": "node memory-testing/index.mjs", "postinstall": "husky install", "postpublish": "pinst --enable" @@ -60,7 +60,7 @@ "taplo format" ], "*.rs": [ - "cargo fmt" + "cargo fmt --" ] }, "husky": {