Merge pull request #965 from godot-rust/feature/local-callable

Bromeon · web-flow · commit aaf74b3dd07e · 2024-12-08T16:12:53.000Z
`Callable::from_local_fn()` + `Callable::from_sync_fn()`
diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs
@@ -7,9 +7,9 @@
 
 use godot_ffi as sys;
 
-use crate::builtin::{inner, StringName, Variant, VariantArray};
+use crate::builtin::{inner, GString, StringName, Variant, VariantArray};
 use crate::classes::Object;
-use crate::meta::{CallContext, GodotType, ToGodot};
+use crate::meta::{AsArg, CallContext, GodotType, ToGodot};
 use crate::obj::bounds::DynMemory;
 use crate::obj::Bounds;
 use crate::obj::{Gd, GodotClass, InstanceId};
@@ -84,43 +84,75 @@ impl Callable {
         }
     }
 
-    /// Create a callable from a Rust function or closure.
+    /// Create callable from **single-threaded** Rust function or closure.
     ///
     /// `name` is used for the string representation of the closure, which helps debugging.
     ///
-    /// Callables created through multiple `from_fn()` calls are never equal, even if they refer to the same function. If you want to use
-    /// equality, either clone an existing `Callable` instance, or define your own `PartialEq` impl with [`Callable::from_custom`].
+    /// This constructor only allows the callable to be invoked from the same thread as creating it. If you need to invoke it from any thread,
+    /// use [`from_sync_fn`][Self::from_sync_fn] instead (requires crate feature `experimental-threads`; only enable if really needed).
+    #[cfg(since_api = "4.2")]
+    pub fn from_local_fn<F, S>(name: S, rust_function: F) -> Self
+    where
+        F: 'static + FnMut(&[&Variant]) -> Result<Variant, ()>,
+        S: AsArg<GString>,
+    {
+        meta::arg_into_owned!(name);
+
+        Self::from_fn_wrapper(FnWrapper {
+            rust_function,
+            name,
+            thread_id: Some(std::thread::current().id()),
+        })
+    }
+
+    /// Create callable from **thread-safe** Rust function or closure.
+    ///
+    /// `name` is used for the string representation of the closure, which helps debugging.
+    ///
+    /// This constructor requires `Send` + `Sync` bound and allows the callable to be invoked from any thread. If you guarantee that you invoke
+    /// it from the same thread as creating it, use [`from_local_fn`][Self::from_local_fn] instead.
+    ///
+    /// Callables created through multiple `from_local_fn` or `from_sync_fn()` calls are never equal, even if they refer to the same function.
+    /// If you want to use equality, either clone an existing `Callable` instance, or define your own `PartialEq` impl with
+    /// [`Callable::from_custom`].
     ///
     /// # Example
     /// ```no_run
     /// # use godot::prelude::*;
-    /// let callable = Callable::from_fn("sum", |args: &[&Variant]| {
+    /// let callable = Callable::from_sync_fn("sum", |args: &[&Variant]| {
     ///     let sum: i32 = args.iter().map(|arg| arg.to::<i32>()).sum();
     ///     Ok(sum.to_variant())
     /// });
     /// ```
     #[cfg(since_api = "4.2")]
-    pub fn from_fn<F, S>(name: S, rust_function: F) -> Self
+    #[cfg(feature = "experimental-threads")]
+    pub fn from_sync_fn<F, S>(name: S, rust_function: F) -> Self
     where
         F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result<Variant, ()>,
-        S: Into<crate::builtin::GString>,
+        S: AsArg<GString>,
     {
-        let userdata = CallableUserdata {
-            inner: FnWrapper {
-                rust_function,
-                name: name.into(),
-            },
-        };
+        meta::arg_into_owned!(name);
 
-        let info = CallableCustomInfo {
-            callable_userdata: Box::into_raw(Box::new(userdata)) as *mut std::ffi::c_void,
-            call_func: Some(rust_callable_call_fn::<F>),
-            free_func: Some(rust_callable_destroy::<FnWrapper<F>>),
-            to_string_func: Some(rust_callable_to_string_named::<F>),
-            ..Self::default_callable_custom_info()
-        };
+        Self::from_fn_wrapper(FnWrapper {
+            rust_function,
+            name,
+            thread_id: None,
+        })
+    }
 
-        Self::from_custom_info(info)
+    #[deprecated = "Now split into from_local_fn (single-threaded) and from_sync_fn (multi-threaded)."]
+    #[cfg(since_api = "4.2")]
+    pub fn from_fn<F, S>(name: S, rust_function: F) -> Self
+    where
+        F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result<Variant, ()>,
+        S: Into<GString>,
+    {
+        // Do not call from_sync_fn() since that is feature-gated, but this isn't due to compatibility.
+        Self::from_fn_wrapper(FnWrapper {
+            rust_function,
+            name: name.into(),
+            thread_id: None,
+        })
     }
 
     /// Create a highly configurable callable from Rust.
@@ -146,6 +178,24 @@ impl Callable {
         Self::from_custom_info(info)
     }
 
+    #[cfg(since_api = "4.2")]
+    fn from_fn_wrapper<F>(inner: FnWrapper<F>) -> Self
+    where
+        F: FnMut(&[&Variant]) -> Result<Variant, ()>,
+    {
+        let userdata = CallableUserdata { inner };
+
+        let info = CallableCustomInfo {
+            callable_userdata: Box::into_raw(Box::new(userdata)) as *mut std::ffi::c_void,
+            call_func: Some(rust_callable_call_fn::<F>),
+            free_func: Some(rust_callable_destroy::<FnWrapper<F>>),
+            to_string_func: Some(rust_callable_to_string_named::<F>),
+            ..Self::default_callable_custom_info()
+        };
+
+        Self::from_custom_info(info)
+    }
+
     #[cfg(since_api = "4.2")]
     fn from_custom_info(mut info: CallableCustomInfo) -> Callable {
         // SAFETY: callable_custom_create() is a valid way of creating callables.
@@ -330,7 +380,7 @@ unsafe impl GodotFfi for Callable {
     }
 }
 
-crate::meta::impl_godot_as_self!(Callable);
+meta::impl_godot_as_self!(Callable);
 
 impl fmt::Debug for Callable {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -365,6 +415,7 @@ mod custom_callable {
     use super::*;
     use crate::builtin::GString;
     use std::hash::Hash;
+    use std::thread::ThreadId;
 
     pub struct CallableUserdata<T> {
         pub inner: T,
@@ -380,8 +431,11 @@ mod custom_callable {
     }
 
     pub(crate) struct FnWrapper<F> {
-        pub(crate) rust_function: F,
-        pub(crate) name: GString,
+        pub(super) rust_function: F,
+        pub(super) name: GString,
+
+        /// `None` if the callable is multi-threaded ([`Callable::from_sync_fn`]).
+        pub(super) thread_id: Option<ThreadId>,
     }
 
     /// Represents a custom callable object defined in Rust.
@@ -419,7 +473,7 @@ mod custom_callable {
             // Get the RustCallable again inside closure so it doesn't have to be UnwindSafe.
             let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata);
             let result = c.invoke(arg_refs);
-            crate::meta::varcall_return_checked(result, r_return, r_error);
+            meta::varcall_return_checked(result, r_return, r_error);
             Ok(())
         });
     }
@@ -444,8 +498,21 @@ mod custom_callable {
         crate::private::handle_varcall_panic(&ctx, &mut *r_error, move || {
             // Get the FnWrapper again inside closure so the FnMut doesn't have to be UnwindSafe.
             let w: &mut FnWrapper<F> = CallableUserdata::inner_from_raw(callable_userdata);
+
+            if w.thread_id
+                .is_some_and(|tid| tid != std::thread::current().id())
+            {
+                // NOTE: this panic is currently not propagated to the caller, but results in an error message and Nil return.
+                // See comments in itest callable_call() for details.
+                panic!(
+                    "Callable '{}' created with from_local_fn() must be called from the same thread it was created in.\n\
+                    If you need to call it from any thread, use from_sync_fn() instead (requires `experimental-threads` feature).",
+                    w.name
+                );
+            }
+
             let result = (w.rust_function)(arg_refs);
-            crate::meta::varcall_return_checked(result, r_return, r_error);
+            meta::varcall_return_checked(result, r_return, r_error);
             Ok(())
         });
     }
diff --git a/godot-ffi/src/binding/single_threaded.rs b/godot-ffi/src/binding/single_threaded.rs
@@ -156,11 +156,23 @@ impl BindingStorage {
                 "Godot engine not available; make sure you are not calling it from unit/doc tests",
             );
 
-            assert_eq!(
-                main_thread_id,
-                std::thread::current().id(),
-                "attempted to access binding from different thread than main thread; this is UB - use the \"experimental-threads\" feature."
-            );
+            if main_thread_id != std::thread::current().id() {
+                // If a binding is accessed the first time, this will panic and start unwinding. It can then happen that during unwinding,
+                // another FFI call happens (e.g. Godot destructor), which would cause immediate abort, swallowing the error message.
+                // Thus check if a panic is already in progress.
+
+                if std::thread::panicking() {
+                    eprintln!(
+                        "ERROR: Attempted to access binding from different thread than main thread; this is UB.\n\
+                        Cannot panic because panic unwind is already in progress. Please check surrounding messages to fix the bug."
+                    );
+                } else {
+                    panic!(
+                        "attempted to access binding from different thread than main thread; \
+                        this is UB - use the \"experimental-threads\" feature."
+                    )
+                };
+            }
         }
 
         // SAFETY: This function can only be called when the binding is initialized and from the main thread, so we know that it's initialized.
diff --git a/itest/rust/src/builtin_tests/containers/array_test.rs b/itest/rust/src/builtin_tests/containers/array_test.rs
@@ -494,7 +494,7 @@ fn array_binary_search_custom() {
 
 #[cfg(since_api = "4.2")]
 fn backwards_sort_callable() -> Callable {
-    Callable::from_fn("sort backwards", |args: &[&Variant]| {
+    Callable::from_local_fn("sort backwards", |args: &[&Variant]| {
         let res = args[0].to::<i32>() > args[1].to::<i32>();
         Ok(res.to_variant())
     })
diff --git a/itest/rust/src/builtin_tests/containers/callable_test.rs b/itest/rust/src/builtin_tests/containers/callable_test.rs
@@ -169,16 +169,84 @@ impl CallableRefcountTest {
 #[cfg(since_api = "4.2")]
 pub mod custom_callable {
     use super::*;
-    use crate::framework::assert_eq_self;
+    use crate::framework::{assert_eq_self, quick_thread, ThreadCrosser};
     use godot::builtin::{Dictionary, RustCallable};
+    use godot::sys;
     use godot::sys::GdextBuild;
     use std::fmt;
     use std::hash::Hash;
     use std::sync::{Arc, Mutex};
 
     #[itest]
-    fn callable_from_fn() {
-        let callable = Callable::from_fn("sum", sum);
+    fn callable_from_local_fn() {
+        let callable = Callable::from_local_fn("sum", sum);
+
+        assert!(callable.is_valid());
+        assert!(!callable.is_null());
+        assert!(callable.is_custom());
+        assert!(callable.object().is_none());
+
+        let sum1 = callable.callv(&varray![1, 2, 4, 8]);
+        assert_eq!(sum1, 15.to_variant());
+
+        // Important to test 0 arguments, as the FFI call passes a null pointer for the argument array.
+        let sum2 = callable.callv(&varray![]);
+        assert_eq!(sum2, 0.to_variant());
+    }
+
+    // Without this feature, any access to the global binding from another thread fails; so the from_local_fn() cannot be tested in isolation.
+    #[itest]
+    fn callable_from_local_fn_crossthread() {
+        // This static is a workaround for not being able to propagate failed `Callable` invocations as panics.
+        // See note in itest callable_call() for further info.
+        static GLOBAL: sys::Global<i32> = sys::Global::default();
+
+        let callable = Callable::from_local_fn("change_global", |_args| {
+            *GLOBAL.lock() = 777;
+            Ok(Variant::nil())
+        });
+
+        // Note that Callable itself isn't Sync/Send, so we have to transfer it unsafely.
+        // Godot may pass it to another thread though without `unsafe`.
+        let crosser = ThreadCrosser::new(callable);
+
+        // Create separate thread and ensure calling fails.
+        // Why expect_panic for (single-threaded && Debug) but not (multi-threaded || Release) mode:
+        // - Check is only enabled in Debug, not Release.
+        // - We currently can't catch panics from Callable invocations, see above. True for both single/multi-threaded.
+        // - In single-threaded mode, there's an FFI access check which panics as soon as another thread is invoked. *This* panics.
+        // - In multi-threaded, we need to observe the effect instead (see below).
+
+        if !cfg!(feature = "experimental-threads") && cfg!(debug_assertions) {
+            // Single-threaded and Debug.
+            crate::framework::expect_panic(
+                "Callable created with from_local_fn() must panic when invoked on other thread",
+                || {
+                    quick_thread(|| {
+                        let callable = unsafe { crosser.extract() };
+                        callable.callv(&varray![5]);
+                    });
+                },
+            );
+        } else {
+            // Multi-threaded OR Release.
+            quick_thread(|| {
+                let callable = unsafe { crosser.extract() };
+                callable.callv(&varray![5]);
+            });
+        }
+
+        assert_eq!(
+            *GLOBAL.lock(),
+            0,
+            "Callable created with from_local_fn() must not run when invoked on other thread"
+        );
+    }
+
+    #[itest]
+    #[cfg(feature = "experimental-threads")]
+    fn callable_from_sync_fn() {
+        let callable = Callable::from_sync_fn("sum", sum);
 
         assert!(callable.is_valid());
         assert!(!callable.is_null());
@@ -199,16 +267,16 @@ pub mod custom_callable {
     #[itest]
     fn callable_custom_with_err() {
         let callable_with_err =
-            Callable::from_fn("on_error_doesnt_crash", |_args: &[&Variant]| Err(()));
+            Callable::from_local_fn("on_error_doesnt_crash", |_args: &[&Variant]| Err(()));
         // Errors in Godot, but should not crash.
         assert_eq!(callable_with_err.callv(&varray![]), Variant::nil());
     }
 
     #[itest]
     fn callable_from_fn_eq() {
-        let a = Callable::from_fn("sum", sum);
+        let a = Callable::from_local_fn("sum", sum);
         let b = a.clone();
-        let c = Callable::from_fn("sum", sum);
+        let c = Callable::from_local_fn("sum", sum);
 
         assert_eq!(a, b, "same function, same instance -> equal");
         assert_ne!(a, c, "same function, different instance -> not equal");
@@ -312,7 +380,7 @@ pub mod custom_callable {
     fn callable_callv_panic_from_fn() {
         let received = Arc::new(AtomicU32::new(0));
         let received_callable = received.clone();
-        let callable = Callable::from_fn("test", move |_args| {
+        let callable = Callable::from_local_fn("test", move |_args| {
             panic!("TEST: {}", received_callable.fetch_add(1, Ordering::SeqCst))
         });
 
diff --git a/itest/rust/src/builtin_tests/containers/signal_test.rs b/itest/rust/src/builtin_tests/containers/signal_test.rs
@@ -247,7 +247,7 @@ mod custom_callable {
     }
 
     fn connect_signal_panic_from_fn(received: Arc<AtomicU32>) -> Callable {
-        Callable::from_fn("test", move |_args| {
+        Callable::from_local_fn("test", move |_args| {
             panic!("TEST: {}", received.fetch_add(1, Ordering::SeqCst))
         })
     }
diff --git a/itest/rust/src/framework/mod.rs b/itest/rust/src/framework/mod.rs

Original file line number	Diff line number	Diff line change
`@@ -247,7 +247,7 @@ mod custom_callable {`
`247`	`247`	`}`
`248`	`248`
`249`	`249`	`fn connect_signal_panic_from_fn(received: Arc<AtomicU32>) -> Callable {`
`250`		`- Callable::from_fn("test", move \|_args\| {`
	`250`	`+ Callable::from_local_fn("test", move \|_args\| {`
`251`	`251`	`panic!("TEST: {}", received.fetch_add(1, Ordering::SeqCst))`
`252`	`252`	`})`
`253`	`253`	`}`