From 0204ef9342d3214068cf1ceb3df3b611c36e9b12 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Tue, 9 Mar 2021 23:24:53 +0100 Subject: [PATCH] Added a way to opt out of error handling for `ThreadsafeFunction`s. This is to allow having the `ThreadSafeFunction` expect the same calling API as an unwrapped `JsFunction` would have: in some contexts this consistency is more desireable than giving the option to the caller to handle a native-to-js conversion failure (or somebody having directly fed a `Result`; although that use case seems oddly niche to me: it can already be covered when `type T = Result;` and propagated/flattened inside the `R` closure with a simple `?`), since such failure can already be quite fatal / unexpected in some cases. In order to keep the code backwards-compatible, this PR uses an added **optional** generic (type) parameter. Since this parameter, semantically, represents an `enum`, and since we don't have yet `const_generic` `enum`s, we circumvent this limitation by using the type-level enum pattern, which is abtracted away with a helper macro that incidentally yields some handy docs (I can attach a rendered version of the docs hosted on netlify if the reviewer so wishes). --- napi/src/threadsafe_function.rs | 276 ++++++++++++++++++++++++++------ 1 file changed, 229 insertions(+), 47 deletions(-) diff --git a/napi/src/threadsafe_function.rs b/napi/src/threadsafe_function.rs index 61ce0c2e..d4c567af 100644 --- a/napi/src/threadsafe_function.rs +++ b/napi/src/threadsafe_function.rs @@ -36,6 +36,68 @@ impl Into for ThreadsafeFunctionCallMode { } } +type_level_enum! { + /// Type-level `enum` to express how to feed [`ThreadsafeFunction`] errors to + /// the inner [`JsFunction`]. + /// + /// ### Context + /// + /// For callbacks that expect a `Result`-like kind of input, the convention is + /// to have the callback take an `error` parameter as its first parameter. + /// + /// This way receiving a `Result` can be modelled as follows: + /// + /// - In case of `Err(error)`, feed that `error` entity as the first parameter + /// of the callback; + /// + /// - Otherwise (in case of `Ok(_)`), feed `null` instead. + /// + /// In pseudo-code: + /// + /// ```rust,ignore + /// match result_args { + /// Ok(args) => { + /// let js_null = /* … */; + /// callback.call( + /// // this + /// None, + /// // args… + /// &iter::once(js_null).chain(args).collect::>(), + /// ) + /// }, + /// Err(err) => callback.call(None, &[JsError::from(err)]), + /// } + /// ``` + /// + /// **Note that the `Err` case can stem from a failed conversion from native + /// values to js values when calling the callback!** + /// + /// That's why: + /// + /// > **[This][`ErrorStrategy::CalleeHandled`] is the default error strategy**. + /// + /// In order to opt-out of it, [`ThreadsafeFunction`] has an optional second + /// generic parameter (of "kind" [`ErrorStrategy::T`]) that defines whether + /// this behavior ([`ErrorStrategy::CalleeHandled`]) or a non-`Result` one + /// ([`ErrorStrategy::Fatal`]) is desired. + pub enum ErrorStrategy { + /// Input errors (including conversion errors) are left for the callee to + /// handle: + /// + /// The callee receives an extra `error` parameter (the first one), which is + /// `null` if no error occurred, and the error payload otherwise. + CalleeHandled, + + /// Input errors (including conversion errors) are deemed fatal: + /// + /// they can thus cause a `panic!` or abort the process. + /// + /// The callee thus is not expected to have to deal with [that extra `error` + /// parameter][CalleeHandled], which is thus not added. + Fatal, + } +} + /// Communicate with the addon's main thread by invoking a JavaScript function from other threads. /// /// ## Example @@ -85,13 +147,13 @@ impl Into for ThreadsafeFunctionCallMode { /// ctx.env.get_undefined() /// } /// ``` -pub struct ThreadsafeFunction { +pub struct ThreadsafeFunction { raw_tsfn: sys::napi_threadsafe_function, aborted: Arc, - _phantom: PhantomData, + _phantom: PhantomData<(T, ES)>, } -impl Clone for ThreadsafeFunction { +impl Clone for ThreadsafeFunction { fn clone(&self) -> Self { if !self.aborted.load(Ordering::Acquire) { let acquire_status = unsafe { sys::napi_acquire_threadsafe_function(self.raw_tsfn) }; @@ -109,14 +171,10 @@ impl Clone for ThreadsafeFunction { } } -unsafe impl Send for ThreadsafeFunction {} -unsafe impl Sync for ThreadsafeFunction {} +unsafe impl Send for ThreadsafeFunction {} +unsafe impl Sync for ThreadsafeFunction {} -struct ThreadSafeContext( - Box) -> Result>>, -); - -impl ThreadsafeFunction { +impl ThreadsafeFunction { /// See [napi_create_threadsafe_function](https://nodejs.org/api/n-api.html#n_api_napi_create_threadsafe_function) /// for more information. #[inline] @@ -139,8 +197,7 @@ impl ThreadsafeFunction { let initial_thread_count = 1usize; let mut raw_tsfn = ptr::null_mut(); - let context = ThreadSafeContext(Box::from(callback)); - let ptr = Box::into_raw(Box::new(context)) as *mut _; + let ptr = Box::into_raw(Box::new(callback)) as *mut _; check_status!(unsafe { sys::napi_create_threadsafe_function( env, @@ -150,9 +207,9 @@ impl ThreadsafeFunction { max_queue_size, initial_thread_count, ptr, - Some(thread_finalize_cb::), + Some(thread_finalize_cb::), ptr, - Some(call_js_cb::), + Some(call_js_cb::), &mut raw_tsfn, ) })?; @@ -164,22 +221,6 @@ impl ThreadsafeFunction { }) } - /// See [napi_call_threadsafe_function](https://nodejs.org/api/n-api.html#n_api_napi_call_threadsafe_function) - /// for more information. - pub fn call(&self, value: Result, mode: ThreadsafeFunctionCallMode) -> Status { - if self.aborted.load(Ordering::Acquire) { - return Status::Closing; - } - unsafe { - sys::napi_call_threadsafe_function( - self.raw_tsfn, - Box::into_raw(Box::new(value)) as *mut _, - mode.into(), - ) - } - .into() - } - /// See [napi_ref_threadsafe_function](https://nodejs.org/api/n-api.html#n_api_napi_ref_threadsafe_function) /// for more information. /// @@ -227,7 +268,43 @@ impl ThreadsafeFunction { } } -impl Drop for ThreadsafeFunction { +impl ThreadsafeFunction { + /// See [napi_call_threadsafe_function](https://nodejs.org/api/n-api.html#n_api_napi_call_threadsafe_function) + /// for more information. + pub fn call(&self, value: Result, mode: ThreadsafeFunctionCallMode) -> Status { + if self.aborted.load(Ordering::Acquire) { + return Status::Closing; + } + unsafe { + sys::napi_call_threadsafe_function( + self.raw_tsfn, + Box::into_raw(Box::new(value)) as *mut _, + mode.into(), + ) + } + .into() + } +} + +impl ThreadsafeFunction { + /// See [napi_call_threadsafe_function](https://nodejs.org/api/n-api.html#n_api_napi_call_threadsafe_function) + /// for more information. + pub fn call(&self, value: T, mode: ThreadsafeFunctionCallMode) -> Status { + if self.aborted.load(Ordering::Acquire) { + return Status::Closing; + } + unsafe { + sys::napi_call_threadsafe_function( + self.raw_tsfn, + Box::into_raw(Box::new(value)) as *mut _, + mode.into(), + ) + } + .into() + } +} + +impl Drop for ThreadsafeFunction { fn drop(&mut self) { if !self.aborted.load(Ordering::Acquire) { let release_status = unsafe { @@ -244,29 +321,37 @@ impl Drop for ThreadsafeFunction { } } -unsafe extern "C" fn thread_finalize_cb( +unsafe extern "C" fn thread_finalize_cb( _raw_env: sys::napi_env, finalize_data: *mut c_void, _finalize_hint: *mut c_void, -) { +) where + R: 'static + Send + FnMut(ThreadSafeCallContext) -> Result>, +{ // cleanup - Box::from_raw(finalize_data as *mut ThreadSafeContext); + drop(Box::::from_raw(finalize_data.cast())); } -unsafe extern "C" fn call_js_cb( +unsafe extern "C" fn call_js_cb( raw_env: sys::napi_env, js_callback: sys::napi_value, context: *mut c_void, data: *mut c_void, -) { +) where + R: 'static + Send + FnMut(ThreadSafeCallContext) -> Result>, + ES: ErrorStrategy::T, +{ + let ctx: &mut R = &mut *context.cast::(); + let val: Result = match ES::VALUE { + ErrorStrategy::CalleeHandled::VALUE => *Box::>::from_raw(data.cast()), + ErrorStrategy::Fatal::VALUE => Ok(*Box::::from_raw(data.cast())), + }; + let mut recv = ptr::null_mut(); sys::napi_get_undefined(raw_env, &mut recv); - let ctx = Box::leak(Box::from_raw(context as *mut ThreadSafeContext)); - let val = Box::from_raw(data as *mut Result); - let ret = val.and_then(|v| { - (ctx.0)(ThreadSafeCallContext { + (ctx)(ThreadSafeCallContext { env: Env::from_raw(raw_env), value: v, }) @@ -279,21 +364,26 @@ unsafe extern "C" fn call_js_cb( // If the Result is an error, pass that as the first argument. match ret { Ok(values) => { - let mut js_null = ptr::null_mut(); - sys::napi_get_null(raw_env, &mut js_null); - let args_length = values.len() + 1; - let mut args: Vec = Vec::with_capacity(args_length); - args.push(js_null); - args.extend(values.iter().map(|v| v.raw())); + let values = values.iter().map(|v| v.raw()); + let args: Vec = if ES::VALUE == ErrorStrategy::CalleeHandled::VALUE { + let mut js_null = ptr::null_mut(); + sys::napi_get_null(raw_env, &mut js_null); + ::core::iter::once(js_null).chain(values).collect() + } else { + values.collect() + }; status = sys::napi_call_function( raw_env, recv, js_callback, - args_length, + args.len(), args.as_ptr(), ptr::null_mut(), ); } + Err(e) if ES::VALUE == ErrorStrategy::Fatal::VALUE => { + panic!("{:?}", e); + } Err(e) => { status = sys::napi_call_function( raw_env, @@ -350,3 +440,95 @@ unsafe extern "C" fn call_js_cb( ); } } + +/// Helper +macro_rules! type_level_enum {( + $( #[doc = $doc:tt] )* + $pub:vis + enum $EnumName:ident { + $( + $( #[doc = $doc_variant:tt] )* + $Variant:ident + ),* $(,)? + } +) => (type_level_enum! { // This requires the macro to be in scope when called. + with_docs! { + $( #[doc = $doc] )* + /// + /// ### Type-level `enum` + /// + /// Until `const_generics` can handle custom `enum`s, this pattern must be + /// implemented at the type level. + /// + /// We thus end up with: + /// + /// ```rust,ignore + /// #[type_level_enum] + #[doc = ::core::concat!( + " enum ", ::core::stringify!($EnumName), " {", + )] + $( + #[doc = ::core::concat!( + " ", ::core::stringify!($Variant), ",", + )] + )* + #[doc = " }"] + /// ``` + /// + #[doc = ::core::concat!( + "With [`", ::core::stringify!($EnumName), "::T`](#reexports) \ + being the type-level \"enum type\":", + )] + /// + /// ```rust,ignore + #[doc = ::core::concat!( + "" + )] + /// ``` + } + #[allow(warnings)] + $pub mod $EnumName { + #[doc(no_inline)] + pub use $EnumName as T; + + super::type_level_enum! { + with_docs! { + #[doc = ::core::concat!( + "See [`", ::core::stringify!($EnumName), "`]\ + [super::", ::core::stringify!($EnumName), "]" + )] + } + pub trait $EnumName : __sealed::$EnumName + ::core::marker::Sized + 'static { + const VALUE: __value::$EnumName; + } + } + + mod __sealed { pub trait $EnumName {} } + + mod __value { + #[derive(Debug, PartialEq, Eq)] + pub enum $EnumName { $( $Variant ),* } + } + + $( + $( #[doc = $doc_variant] )* + pub enum $Variant {} + impl __sealed::$EnumName for $Variant {} + impl $EnumName for $Variant { + const VALUE: __value::$EnumName = __value::$EnumName::$Variant; + } + impl $Variant { + pub const VALUE: __value::$EnumName = __value::$EnumName::$Variant; + } + )* + } +});( + with_docs! { + $( #[doc = $doc:expr] )* + } + $item:item +) => ( + $( #[doc = $doc] )* + $item +)} +use type_level_enum;