Merge pull request #873 from jrb0001/callable-panic

Don't abort on panic inside Callable

Author: Bromeon

godot-core/src/builtin/callable.rs

@@ -9,7 +9,7 @@ use godot_ffi as sys;
 
 use crate::builtin::{inner, StringName, Variant, VariantArray};
 use crate::classes::Object;
-use crate::meta::{GodotType, ToGodot};
+use crate::meta::{CallContext, GodotType, ToGodot};
 use crate::obj::bounds::DynMemory;
 use crate::obj::Bounds;
 use crate::obj::{Gd, GodotClass, InstanceId};
@@ -392,10 +392,19 @@ mod custom_callable {
     ) {
         let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize);
 
-        let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata);
+        let name = {
+            let c: &C = CallableUserdata::inner_from_raw(callable_userdata);
+            c.to_string()
+        };
+        let ctx = CallContext::custom_callable(name.as_str());
 
-        let result = c.invoke(arg_refs);
-        crate::meta::varcall_return_checked(result, r_return, r_error);
+        crate::private::handle_varcall_panic(&ctx, &mut *r_error, move || {
+            // 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);
+            Ok(())
+        });
     }
 
     pub unsafe extern "C" fn rust_callable_call_fn<F>(
@@ -409,10 +418,19 @@ mod custom_callable {
     {
         let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize);
 
-        let w: &mut FnWrapper<F> = CallableUserdata::inner_from_raw(callable_userdata);
-
-        let result = (w.rust_function)(arg_refs);
-        crate::meta::varcall_return_checked(result, r_return, r_error);
+        let name = {
+            let w: &FnWrapper<F> = CallableUserdata::inner_from_raw(callable_userdata);
+            w.name.to_string()
+        };
+        let ctx = CallContext::custom_callable(name.as_str());
+
+        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);
+            let result = (w.rust_function)(arg_refs);
+            crate::meta::varcall_return_checked(result, r_return, r_error);
+            Ok(())
+        });
     }
 
     pub unsafe extern "C" fn rust_callable_destroy<T>(callable_userdata: *mut std::ffi::c_void) {

godot-core/src/meta/signature.rs

@@ -627,6 +627,14 @@ impl<'a> CallContext<'a> {
         }
     }
 
+    /// Call from Godot into a custom Callable.
+    pub fn custom_callable(function_name: &'a str) -> Self {
+        Self {
+            class_name: Cow::Borrowed("<Callable>"),
+            function_name,
+        }
+    }
+
     /// Outbound call from Rust into the engine, class/builtin APIs.
     pub const fn outbound(class_name: &'a str, function_name: &'a str) -> Self {
         Self {

itest/rust/src/builtin_tests/containers/callable_test.rs

@@ -4,13 +4,15 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
-
 use godot::builtin::inner::InnerCallable;
 use godot::builtin::{varray, Callable, GString, StringName, Variant};
 use godot::classes::{Node2D, Object};
 use godot::meta::ToGodot;
 use godot::obj::{NewAlloc, NewGd};
 use godot::register::{godot_api, GodotClass};
+use std::fmt::{Display, Formatter};
+use std::hash::Hasher;
+use std::sync::atomic::{AtomicU32, Ordering};
 
 use crate::framework::itest;
 
@@ -159,10 +161,10 @@ impl CallableRefcountTest {
 // Tests and infrastructure for custom callables
 
 #[cfg(since_api = "4.2")]
-mod custom_callable {
+pub mod custom_callable {
     use super::*;
     use crate::framework::assert_eq_self;
-    use godot::builtin::Dictionary;
+    use godot::builtin::{Dictionary, RustCallable};
     use std::fmt;
     use std::hash::Hash;
     use std::sync::{Arc, Mutex};
@@ -287,6 +289,29 @@ mod custom_callable {
         assert_eq!(eq_count(&bt), 1, "hash collision, eq for b needed");
     }
 
+    #[itest]
+    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| {
+            panic!("TEST: {}", received_callable.fetch_add(1, Ordering::SeqCst))
+        });
+
+        assert_eq!(Variant::nil(), callable.callv(varray![]));
+
+        assert_eq!(1, received.load(Ordering::SeqCst));
+    }
+
+    #[itest]
+    fn callable_callv_panic_from_custom() {
+        let received = Arc::new(AtomicU32::new(0));
+        let callable = Callable::from_custom(PanicCallable(received.clone()));
+
+        assert_eq!(Variant::nil(), callable.callv(varray![]));
+
+        assert_eq!(1, received.load(Ordering::SeqCst));
+    }
+
     struct Adder {
         sum: i32,
 
@@ -365,6 +390,32 @@ mod custom_callable {
     fn hash_count(tracker: &Arc<Mutex<Tracker>>) -> usize {
         tracker.lock().unwrap().hash_counter
     }
+
+    pub struct PanicCallable(pub Arc<AtomicU32>);
+
+    impl PartialEq for PanicCallable {
+        fn eq(&self, other: &Self) -> bool {
+            Arc::ptr_eq(&self.0, &other.0)
+        }
+    }
+
+    impl Hash for PanicCallable {
+        fn hash<H: Hasher>(&self, state: &mut H) {
+            state.write_usize(Arc::as_ptr(&self.0) as usize)
+        }
+    }
+
+    impl Display for PanicCallable {
+        fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+            write!(f, "test")
+        }
+    }
+
+    impl RustCallable for PanicCallable {
+        fn invoke(&mut self, _args: &[&Variant]) -> Result<Variant, ()> {
+            panic!("TEST: {}", self.0.fetch_add(1, Ordering::SeqCst))
+        }
+    }
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

itest/rust/src/builtin_tests/containers/signal_test.rs

@@ -5,11 +5,10 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
-use std::cell::Cell;
-
 use godot::builtin::{Callable, GString, Signal, StringName, Variant};
 use godot::meta::ToGodot;
 use godot::register::{godot_api, GodotClass};
+use std::cell::Cell;
 
 use godot::classes::{Object, RefCounted};
 use godot::obj::{Base, Gd, NewAlloc, NewGd, WithBaseField};
@@ -149,3 +148,155 @@ fn connect_signal() {
 
     receiver.free();
 }
+
+#[cfg(since_api = "4.2")]
+mod custom_callable {
+    use godot::builtin::{Callable, Signal, StringName};
+    use godot::meta::ToGodot;
+    use std::sync::atomic::{AtomicU32, Ordering};
+    use std::sync::Arc;
+
+    use godot::classes::Node;
+    use godot::obj::{Gd, NewAlloc};
+
+    use crate::builtin_tests::containers::callable_test::custom_callable::PanicCallable;
+    use crate::framework::{itest, TestContext};
+
+    #[itest]
+    fn connect_signal_panic_user_from_fn() {
+        connect_signal_panic_shared(
+            "test_signal",
+            connect_signal_panic_from_fn,
+            |node| {
+                node.add_user_signal("test_signal".into());
+            },
+            |node| {
+                node.emit_signal(StringName::from("test_signal"), &[987i64.to_variant()]);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_user_from_custom() {
+        connect_signal_panic_shared(
+            "test_signal",
+            connect_signal_panic_from_custom,
+            |node| {
+                node.add_user_signal("test_signal".into());
+            },
+            |node| {
+                node.emit_signal(StringName::from("test_signal"), &[987i64.to_variant()]);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_fn_tree_entered(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_entered",
+            connect_signal_panic_from_fn,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_custom_tree_entered(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_entered",
+            connect_signal_panic_from_custom,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_fn_tree_exiting(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_exiting",
+            connect_signal_panic_from_fn,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_custom_tree_exiting(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_exiting",
+            connect_signal_panic_from_custom,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_fn_tree_exited(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_exited",
+            connect_signal_panic_from_fn,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    #[itest]
+    fn connect_signal_panic_from_custom_tree_exited(ctx: &TestContext) {
+        connect_signal_panic_shared(
+            "tree_exited",
+            connect_signal_panic_from_custom,
+            |_node| {},
+            |node| {
+                ctx.scene_tree.clone().add_child(&*node);
+                ctx.scene_tree.clone().remove_child(&*node);
+            },
+        );
+    }
+
+    fn connect_signal_panic_shared(
+        signal: &str,
+        callable: impl FnOnce(Arc<AtomicU32>) -> Callable,
+        before: impl FnOnce(&mut Gd<Node>),
+        emit: impl FnOnce(&mut Gd<Node>),
+    ) {
+        let mut node = Node::new_alloc();
+        before(&mut node);
+
+        let signal = Signal::from_object_signal(&node, signal);
+
+        let received = Arc::new(AtomicU32::new(0));
+        let callable = callable(received.clone());
+        signal.connect(callable, 0);
+
+        emit(&mut node);
+
+        assert_eq!(1, received.load(Ordering::SeqCst));
+
+        node.free();
+    }
+
+    fn connect_signal_panic_from_fn(received: Arc<AtomicU32>) -> Callable {
+        Callable::from_fn("test", move |_args| {
+            panic!("TEST: {}", received.fetch_add(1, Ordering::SeqCst))
+        })
+    }
+
+    fn connect_signal_panic_from_custom(received: Arc<AtomicU32>) -> Callable {
+        Callable::from_custom(PanicCallable(received))
+    }
+}