diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index d69dd6d72..b82f6bcf1 100644 --- 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(name: S, rust_function: F) -> Self + where + F: 'static + FnMut(&[&Variant]) -> Result, + S: AsArg, + { + 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::()).sum(); /// Ok(sum.to_variant()) /// }); /// ``` #[cfg(since_api = "4.2")] - pub fn from_fn(name: S, rust_function: F) -> Self + #[cfg(feature = "experimental-threads")] + pub fn from_sync_fn(name: S, rust_function: F) -> Self where F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result, - S: Into, + S: AsArg, { - 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::), - free_func: Some(rust_callable_destroy::>), - to_string_func: Some(rust_callable_to_string_named::), - ..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(name: S, rust_function: F) -> Self + where + F: 'static + Send + Sync + FnMut(&[&Variant]) -> Result, + S: Into, + { + // 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(inner: FnWrapper) -> Self + where + F: FnMut(&[&Variant]) -> Result, + { + 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::), + free_func: Some(rust_callable_destroy::>), + to_string_func: Some(rust_callable_to_string_named::), + ..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 { pub inner: T, @@ -380,8 +431,11 @@ mod custom_callable { } pub(crate) struct FnWrapper { - 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, } /// 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 = 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 index d7324ed84..d3ca17eba 100644 --- 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 index 24859ba23..bfebf8fc0 100644 --- 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::() > args[1].to::(); 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 index 19a221fdf..9408bb0e7 100644 --- 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 = 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 index f6ebcad7d..bf13b277e 100644 --- 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) -> 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 index 4bc0fdfe7..703a1eed0 100644 --- a/itest/rust/src/framework/mod.rs +++ b/itest/rust/src/framework/mod.rs @@ -77,6 +77,25 @@ pub struct TestContext { pub property_tests: Gd, } +/// Utility to assert that something can be sent between threads. +pub struct ThreadCrosser { + value: T, +} + +impl ThreadCrosser { + pub fn new(value: T) -> Self { + Self { value } + } + + /// # Safety + /// Bypasses `Send` checks, user's responsibility. + pub unsafe fn extract(self) -> T { + self.value + } +} + +unsafe impl Send for ThreadCrosser {} + #[derive(Copy, Clone)] pub struct RustTestCase { pub name: &'static str, @@ -125,6 +144,29 @@ pub fn expect_panic(context: &str, code: impl FnOnce()) { ); } +/// Synchronously run a thread and return result. Panics are propagated to caller thread. +#[track_caller] +pub fn quick_thread(f: F) -> R +where + F: FnOnce() -> R + Send + 'static, + R: Send + 'static, +{ + let handle = std::thread::spawn(f); + + match handle.join() { + Ok(result) => result, + Err(panic_payload) => { + if let Some(s) = panic_payload.downcast_ref::<&str>() { + panic!("quick_thread panicked: {s}") + } else if let Some(s) = panic_payload.downcast_ref::() { + panic!("quick_thread panicked: {s}") + } else { + panic!("quick_thread panicked with unknown type.") + }; + } + } +} + /// Disable printing errors from Godot. Ideally we should catch and handle errors, ensuring they happen when /// expected. But that isn't possible, so for now we can just disable printing the error to avoid spamming /// the terminal when tests should error.