Merge pull request #979 from neon-bindings/kv/unnecessary-send

Relax Send constraints

Author: kjvalencik

Cargo.lock

@@ -31,6 +31,7 @@ neon-macros = { version = "=1.0.0-alpha.2", path = "../neon-macros" }
 aquamarine = { version = "0.1.11", optional = true }
 easy-cast = { version = "0.5.1", optional = true }
 doc-comment = { version = "0.3.3", optional = true }
+send_wrapper = "0.6"
 
 [dependencies.tokio]
 version = "1.24.2"

crates/neon/Cargo.toml

@@ -423,7 +423,7 @@ pub trait Context<'a>: ContextInternal<'a> {
     ///     Ok(point)
     /// }
     /// ```
-    fn boxed<U: Finalize + Send + 'static>(&mut self, v: U) -> Handle<'a, JsBox<U>> {
+    fn boxed<U: Finalize + 'static>(&mut self, v: U) -> Handle<'a, JsBox<U>> {
         JsBox::new(self, v)
     }
 

crates/neon/src/context/mod.rs

@@ -48,7 +48,7 @@ where
     /// of the `execute` callback
     pub fn and_then<F>(self, complete: F)
     where
-        F: FnOnce(TaskContext, O) -> NeonResult<()> + Send + 'static,
+        F: FnOnce(TaskContext, O) -> NeonResult<()> + 'static,
     {
         let env = self.cx.env();
         let execute = self.execute;
@@ -65,7 +65,7 @@ where
     pub fn promise<V, F>(self, complete: F) -> Handle<'a, JsPromise>
     where
         V: Value,
-        F: FnOnce(TaskContext, O) -> JsResult<V> + Send + 'static,
+        F: FnOnce(TaskContext, O) -> JsResult<V> + 'static,
     {
         let env = self.cx.env();
         let (deferred, promise) = JsPromise::new(self.cx);
@@ -82,7 +82,7 @@ fn schedule<I, O, D>(env: Env, input: I, data: D)
 where
     I: FnOnce() -> O + Send + 'static,
     O: Send + 'static,
-    D: FnOnce(TaskContext, O) -> NeonResult<()> + Send + 'static,
+    D: FnOnce(TaskContext, O) -> NeonResult<()> + 'static,
 {
     unsafe {
         async_work::schedule(env.to_raw(), input, execute::<I, O>, complete::<O, D>, data);
@@ -100,7 +100,7 @@ where
 fn complete<O, D>(env: raw::Env, output: thread::Result<O>, callback: D)
 where
     O: Send + 'static,
-    D: FnOnce(TaskContext, O) -> NeonResult<()> + Send + 'static,
+    D: FnOnce(TaskContext, O) -> NeonResult<()> + 'static,
 {
     let output = output.unwrap_or_else(|panic| {
         // If a panic was caught while executing the task on the Node Worker
@@ -118,7 +118,7 @@ fn schedule_promise<I, O, D, V>(env: Env, input: I, complete: D, deferred: Defer
 where
     I: FnOnce() -> O + Send + 'static,
     O: Send + 'static,
-    D: FnOnce(TaskContext, O) -> JsResult<V> + Send + 'static,
+    D: FnOnce(TaskContext, O) -> JsResult<V> + 'static,
     V: Value,
 {
     unsafe {
@@ -138,7 +138,7 @@ fn complete_promise<O, D, V>(
     (complete, deferred): (D, Deferred),
 ) where
     O: Send + 'static,
-    D: FnOnce(TaskContext, O) -> JsResult<V> + Send + 'static,
+    D: FnOnce(TaskContext, O) -> JsResult<V> + 'static,
     V: Value,
 {
     let env = env.into();

crates/neon/src/event/task.rs

@@ -14,7 +14,9 @@ use std::{
     ptr, thread,
 };
 
-use super::{bindings as napi, no_panic::FailureBoundary, raw::Env};
+use super::{
+    bindings as napi, debug_send_wrapper::DebugSendWrapper, no_panic::FailureBoundary, raw::Env,
+};
 
 const BOUNDARY: FailureBoundary = FailureBoundary {
     both: "A panic and exception occurred while executing a `neon::event::TaskBuilder` task",
@@ -40,13 +42,13 @@ pub unsafe fn schedule<I, O, D>(
 ) where
     I: Send + 'static,
     O: Send + 'static,
-    D: Send + 'static,
+    D: 'static,
 {
     let mut data = Box::new(Data {
         state: State::Input(input),
         execute,
         complete,
-        data,
+        data: DebugSendWrapper::new(data),
         // Work is initialized as a null pointer, but set by `create_async_work`
         // `data` must not be used until this value has been set.
         work: ptr::null_mut(),
@@ -85,7 +87,7 @@ struct Data<I, O, D> {
     state: State<I, O>,
     execute: Execute<I, O>,
     complete: Complete<O, D>,
-    data: D,
+    data: DebugSendWrapper<D>,
     work: napi::AsyncWork,
 }
 
@@ -170,7 +172,7 @@ unsafe extern "C" fn call_complete<I, O, D>(env: Env, status: napi::Status, data
         };
 
         match status {
-            napi::Status::Ok => complete(env, output, data),
+            napi::Status::Ok => complete(env, output, data.take()),
             napi::Status::Cancelled => {}
             _ => assert_eq!(status, napi::Status::Ok),
         }

crates/neon/src/sys/async_work.rs

@@ -0,0 +1,56 @@
+//! Wrapper that ensures types are always used from the same thread
+//! in debug builds. It is a zero-cost in release builds.
+
+pub(super) use wrapper::DebugSendWrapper;
+
+#[cfg(debug_assertions)]
+mod wrapper {
+    use std::ops::Deref;
+
+    #[repr(transparent)]
+    pub struct DebugSendWrapper<T>(send_wrapper::SendWrapper<T>);
+
+    impl<T> DebugSendWrapper<T> {
+        pub fn new(value: T) -> Self {
+            Self(send_wrapper::SendWrapper::new(value))
+        }
+
+        pub fn take(self) -> T {
+            self.0.take()
+        }
+    }
+
+    impl<T> Deref for DebugSendWrapper<T> {
+        type Target = T;
+
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+}
+
+#[cfg(not(debug_assertions))]
+mod wrapper {
+    use std::ops::Deref;
+
+    #[repr(transparent)]
+    pub struct DebugSendWrapper<T>(T);
+
+    impl<T> DebugSendWrapper<T> {
+        pub fn new(value: T) -> Self {
+            Self(value)
+        }
+
+        pub fn take(self) -> T {
+            self.0
+        }
+    }
+
+    impl<T> Deref for DebugSendWrapper<T> {
+        type Target = T;
+
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+}

crates/neon/src/sys/debug_send_wrapper.rs

@@ -2,11 +2,12 @@ use std::mem::MaybeUninit;
 
 use super::{
     bindings as napi,
+    debug_send_wrapper::DebugSendWrapper,
     raw::{Env, Local},
 };
 
 /// `finalize_external` is invoked immediately before a `napi_external` is garbage collected
-extern "C" fn finalize_external<T: Send + 'static>(
+extern "C" fn finalize_external<T: 'static>(
     env: Env,
     // Raw pointer to a `Box<T>` stored by a `napi_external`
     data: *mut std::ffi::c_void,
@@ -15,10 +16,10 @@ extern "C" fn finalize_external<T: Send + 'static>(
     hint: *mut std::ffi::c_void,
 ) {
     unsafe {
-        let data = Box::<T>::from_raw(data as *mut _);
+        let data = Box::<DebugSendWrapper<T>>::from_raw(data as *mut _);
         let finalizer: fn(Env, T) = std::mem::transmute(hint as *const ());
 
-        finalizer(env, *data);
+        finalizer(env, data.take());
     }
 }
 
@@ -27,7 +28,7 @@ extern "C" fn finalize_external<T: Send + 'static>(
 /// module. Calling `deref` with an external created by another native module,
 /// even another neon module, is undefined behavior.
 /// <https://github.com/neon-bindings/neon/issues/591>
-pub unsafe fn deref<T: Send + 'static>(env: Env, local: Local) -> Option<*const T> {
+pub unsafe fn deref<T: 'static>(env: Env, local: Local) -> Option<*const T> {
     let mut result = MaybeUninit::uninit();
     let status = napi::typeof_value(env, local, result.as_mut_ptr());
 
@@ -53,12 +54,15 @@ pub unsafe fn deref<T: Send + 'static>(env: Env, local: Local) -> Option<*const
 
     assert_eq!(status, napi::Status::Ok);
 
-    Some(result.assume_init() as *const _)
+    let v = result.assume_init();
+    let v = &**v.cast_const().cast::<DebugSendWrapper<T>>() as *const T;
+
+    Some(v)
 }
 
 /// Creates a `napi_external` from a Rust type
-pub unsafe fn create<T: Send + 'static>(env: Env, v: T, finalizer: fn(Env, T)) -> Local {
-    let v = Box::new(v);
+pub unsafe fn create<T: 'static>(env: Env, v: T, finalizer: fn(Env, T)) -> Local {
+    let v = Box::new(DebugSendWrapper::new(v));
     let mut result = MaybeUninit::uninit();
 
     let status = napi::create_external(

crates/neon/src/sys/external.rs

@@ -100,6 +100,7 @@ pub(crate) mod tsfn;
 #[cfg(feature = "napi-5")]
 pub(crate) mod date;
 
+mod debug_send_wrapper;
 #[cfg(feature = "napi-6")]
 pub(crate) mod lifecycle;
 

crates/neon/src/sys/mod.rs

@@ -18,6 +18,7 @@ use std::{
 
 use super::{
     bindings as napi,
+    debug_send_wrapper::DebugSendWrapper,
     error::fatal_error,
     raw::{Env, Local},
 };
@@ -256,7 +257,7 @@ unsafe fn external_from_panic(env: Env, panic: Panic) -> Local {
     let mut result = MaybeUninit::uninit();
     let status = napi::create_external(
         env,
-        Box::into_raw(Box::new(panic)).cast(),
+        Box::into_raw(Box::new(DebugSendWrapper::new(panic))).cast(),
         Some(finalize_panic),
         ptr::null_mut(),
         result.as_mut_ptr(),

crates/neon/src/sys/no_panic.rs

@@ -11,10 +11,10 @@ use crate::{
     types::{boxed::private::JsBoxInner, private::ValueInternal, Value},
 };
 
-type BoxAny = Box<dyn Any + Send + 'static>;
+type BoxAny = Box<dyn Any + 'static>;
 
 mod private {
-    pub struct JsBoxInner<T: Send + 'static> {
+    pub struct JsBoxInner<T: 'static> {
         pub(super) local: crate::sys::raw::Local,
         // Cached raw pointer to the data contained in the `JsBox`. This value is
         // required to implement `Deref` for `JsBox`. Unlike most `Js` types, `JsBox`
@@ -141,15 +141,15 @@ mod private {
 ///     Ok(cx.string(greeting))
 /// }
 #[repr(transparent)]
-pub struct JsBox<T: Send + 'static>(JsBoxInner<T>);
+pub struct JsBox<T: 'static>(JsBoxInner<T>);
 
-impl<T: Send + 'static> std::fmt::Debug for JsBoxInner<T> {
+impl<T: 'static> std::fmt::Debug for JsBoxInner<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(f, "JsBox<{}>", std::any::type_name::<T>())
     }
 }
 
-impl<T: Send + 'static> std::fmt::Debug for JsBox<T> {
+impl<T: 'static> std::fmt::Debug for JsBox<T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         std::fmt::Debug::fmt(&self.0, f)
     }
@@ -162,7 +162,7 @@ unsafe fn maybe_external_deref<'a>(env: Env, local: raw::Local) -> Option<&'a Bo
 }
 
 // Custom `Clone` implementation since `T` might not be `Clone`
-impl<T: Send + 'static> Clone for JsBoxInner<T> {
+impl<T: 'static> Clone for JsBoxInner<T> {
     fn clone(&self) -> Self {
         Self {
             local: self.local,
@@ -171,21 +171,21 @@ impl<T: Send + 'static> Clone for JsBoxInner<T> {
     }
 }
 
-impl<T: Send + 'static> Object for JsBox<T> {}
+impl<T: 'static> Object for JsBox<T> {}
 
-impl<T: Send + 'static> Copy for JsBoxInner<T> {}
+impl<T: 'static> Copy for JsBoxInner<T> {}
 
-impl<T: Send + 'static> Value for JsBox<T> {}
+impl<T: 'static> Value for JsBox<T> {}
 
-unsafe impl<T: Send + 'static> TransparentNoCopyWrapper for JsBox<T> {
+unsafe impl<T: 'static> TransparentNoCopyWrapper for JsBox<T> {
     type Inner = JsBoxInner<T>;
 
     fn into_inner(self) -> Self::Inner {
         self.0
     }
 }
 
-impl<T: Send + 'static> ValueInternal for JsBox<T> {
+impl<T: 'static> ValueInternal for JsBox<T> {
     fn name() -> String {
         any::type_name::<Self>().to_string()
     }
@@ -219,30 +219,25 @@ impl<T: Send + 'static> ValueInternal for JsBox<T> {
     }
 }
 
-/// Values contained by a `JsBox` must be `Finalize + Send + 'static`
+/// Values contained by a `JsBox` must be `Finalize + 'static`
 ///
 /// ### `Finalize`
 ///
 /// The `sys::prelude::Finalize` trait provides a `finalize` method that will be called
 /// immediately before the `JsBox` is garbage collected.
 ///
-/// ### `Send`
-///
-/// `JsBox` may be moved across threads. It is important to guarantee that the
-/// contents is also safe to move across threads.
-///
 /// ### `'static'
 ///
 /// The lifetime of a `JsBox` is managed by the JavaScript garbage collector. Since Rust
 /// is unable to verify the lifetime of the contents, references must be valid for the
 /// entire duration of the program. This does not mean that the `JsBox` will be valid
 /// until the application terminates, only that its lifetime is indefinite.
-impl<T: Finalize + Send + 'static> JsBox<T> {
+impl<T: Finalize + 'static> JsBox<T> {
     /// Constructs a new `JsBox` containing `value`.
     pub fn new<'a, C>(cx: &mut C, value: T) -> Handle<'a, JsBox<T>>
     where
         C: Context<'a>,
-        T: Send + 'static,
+        T: 'static,
     {
         // This function will execute immediately before the `JsBox` is garbage collected.
         // It unwraps the `napi_external`, downcasts the `BoxAny` and moves the type
@@ -264,7 +259,7 @@ impl<T: Finalize + Send + 'static> JsBox<T> {
     }
 }
 
-impl<T: Send + 'static> Deref for JsBox<T> {
+impl<T: 'static> Deref for JsBox<T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {