Panic handling: thread safety; set hook once and not repeatedly

godot-core/src/init/mod.rs

@@ -46,6 +46,17 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
             sys::initialize(get_proc_address, library, config);
         }
 
+        // With experimental-features enabled, we can always print panics to godot_print!
+        #[cfg(feature = "experimental-threads")]
+        crate::private::set_gdext_hook(|| true);
+
+        #[cfg(not(feature = "experimental-threads"))]
+        {
+            let main_thread = std::thread::current().id();
+            // Without experimental-features enabled, we can only print panics with godot_print! if the panic occurs on the main (godot) thread
+            crate::private::set_gdext_hook(move || std::thread::current().id() == main_thread);
+        }
+
         // Currently no way to express failure; could be exposed to E if necessary.
         // No early exit, unclear if Godot still requires output parameters to be set.
         let success = true;
@@ -65,7 +76,7 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
         success as u8
     };
 
-    let ctx = || "error when loading GDExtension library";
+    let ctx = || "error when loading GDExtension library".to_string();
     let is_success = crate::private::handle_panic(ctx, init_code);
 
     is_success.unwrap_or(0)

godot-core/src/lib.rs

@@ -30,6 +30,7 @@ pub mod registry;
 pub mod tools;
 
 mod storage;
+pub use crate::private::{get_gdext_panic_context, set_gdext_hook};
 pub use godot_ffi as sys;
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

godot-core/src/private.rs

@@ -22,9 +22,9 @@ use crate::global::godot_error;
 use crate::meta::error::CallError;
 use crate::meta::CallContext;
 use crate::sys;
+use std::cell::RefCell;
+use std::io::Write;
 use std::sync::atomic;
-#[cfg(debug_assertions)]
-use std::sync::{Arc, Mutex};
 use sys::Global;
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
@@ -180,11 +180,6 @@ pub unsafe fn has_virtual_script_method(
     sys::interface_fn!(object_has_script_method)(sys::to_const_ptr(object_ptr), method_sname) != 0
 }
 
-pub fn flush_stdout() {
-    use std::io::Write;
-    std::io::stdout().flush().expect("flush stdout");
-}
-
 /// Ensure `T` is an editor plugin.
 pub const fn is_editor_plugin<T: crate::obj::Inherits<crate::classes::EditorPlugin>>() {}
 
@@ -221,15 +216,7 @@ pub fn is_class_runtime(is_tool: bool) -> bool {
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Panic handling
 
-#[cfg(debug_assertions)]
-#[derive(Debug)]
-struct GodotPanicInfo {
-    line: u32,
-    file: String,
-    //backtrace: Backtrace, // for future use
-}
-
-pub fn extract_panic_message(err: Box<dyn std::any::Any + Send>) -> String {
+pub fn extract_panic_message(err: &(dyn Send + std::any::Any)) -> String {
     if let Some(s) = err.downcast_ref::<&'static str>() {
         s.to_string()
     } else if let Some(s) = err.downcast_ref::<String>() {
@@ -239,18 +226,45 @@ pub fn extract_panic_message(err: Box<dyn std::any::Any + Send>) -> String {
     }
 }
 
-fn format_panic_message(msg: String) -> String {
+fn format_panic_message(_location: Option<&std::panic::Location<'_>>, mut msg: String) -> String {
+    if let Some(context) = get_gdext_panic_context() {
+        msg = format!("{msg}\nContext: {context}");
+    }
+
+    let prefix = if let Some(location) = _location {
+        format!("panic {}:{}", location.file(), location.line())
+    } else {
+        "panic".to_string()
+    };
+
     // If the message contains newlines, print all of the lines after a line break, and indent them.
     let lbegin = "\n  ";
     let indented = msg.replace('\n', lbegin);
 
     if indented.len() != msg.len() {
-        format!("[panic]{lbegin}{indented}")
+        format!("[{prefix}]{lbegin}{indented}")
     } else {
-        format!("[panic]  {msg}")
+        format!("[{prefix}]  {msg}")
     }
 }
 
+pub fn set_gdext_hook(godot_print: impl 'static + Send + Sync + Fn() -> bool) {
+    std::panic::set_hook(Box::new(move |panic_info| {
+        // Flush, to make sure previous Rust output (e.g. test announcement, or debug prints during app) have been printed
+        let _ignored_result = std::io::stdout().flush();
+
+        let message = extract_panic_message(panic_info.payload());
+        let message = format_panic_message(panic_info.location(), message);
+        if godot_print() {
+            godot_error!("{message}");
+        }
+        eprintln!("{message}");
+        #[cfg(debug_assertions)]
+        eprintln!("{}", std::backtrace::Backtrace::capture());
+        let _ignored_result = std::io::stderr().flush();
+    }));
+}
+
 pub fn set_error_print_level(level: u8) -> u8 {
     assert!(level <= 2);
     ERROR_PRINT_LEVEL.swap(level, atomic::Ordering::Relaxed)
@@ -261,19 +275,76 @@ pub(crate) fn has_error_print_level(level: u8) -> bool {
     ERROR_PRINT_LEVEL.load(atomic::Ordering::Relaxed) >= level
 }
 
+#[cfg(debug_assertions)]
+struct ScopedFunctionStack(Vec<*const dyn Fn() -> String>);
+
+#[cfg(debug_assertions)]
+impl ScopedFunctionStack {
+    /// # Safety
+    /// Function must removed (using pop_function) before lifetime is invalidated.
+    unsafe fn push_function(&mut self, function: &dyn Fn() -> String) {
+        /// # Safety
+        /// The caller must ensure that the function isn't used past its original lifetime.
+        /// (Invariant must be held by push_function)
+        unsafe fn assume_static_lifetime(
+            value: &dyn Fn() -> String,
+        ) -> &'static dyn Fn() -> String {
+            std::mem::transmute(value)
+        }
+        self.0.push(assume_static_lifetime(function) as *const _);
+    }
+
+    fn pop_function(&mut self) {
+        self.0.pop();
+    }
+
+    fn get_last(&self) -> Option<String> {
+        self.0.last().cloned().map(|pointer| unsafe {
+            // SAFETY:
+            // Invariants provided by push_function assert that any and all functions held by ScopedFunctionStack
+            // are removed before they are invalidated; functions must always be valid
+            &*pointer
+        }())
+    }
+}
+
+#[cfg(debug_assertions)]
+thread_local! {
+    static ERROR_CONTEXT_STACK: RefCell<ScopedFunctionStack> = const {
+        RefCell::new(ScopedFunctionStack(Vec::new()))
+    }
+}
+
+// Value may return `None` even from panic hook if called from non-godot thread
+pub fn get_gdext_panic_context() -> Option<String> {
+    #[cfg(debug_assertions)]
+    return ERROR_CONTEXT_STACK.with(|cell| cell.borrow().get_last());
+    #[cfg(not(debug_assertions))]
+    None
+}
+
 /// Executes `code`. If a panic is thrown, it is caught and an error message is printed to Godot.
 ///
 /// Returns `Err(message)` if a panic occurred, and `Ok(result)` with the result of `code` otherwise.
 ///
 /// In contrast to [`handle_varcall_panic`] and [`handle_ptrcall_panic`], this function is not intended for use in `try_` functions,
 /// where the error is propagated as a `CallError` in a global variable.
-pub fn handle_panic<E, F, R, S>(error_context: E, code: F) -> Result<R, String>
+pub fn handle_panic<E, F, R>(error_context: E, code: F) -> Result<R, String>
 where
-    E: FnOnce() -> S,
+    E: Fn() -> String,
     F: FnOnce() -> R + std::panic::UnwindSafe,
-    S: std::fmt::Display,
 {
-    handle_panic_with_print(error_context, code, has_error_print_level(1))
+    #[cfg(debug_assertions)]
+    ERROR_CONTEXT_STACK.with(|cell| unsafe {
+        // SAFETY:
+        // &error_context is valid for lifetime of function, and is removed from LAST_ERROR_CONTEXT before end of function
+        cell.borrow_mut().push_function(&error_context)
+    });
+    let result =
+        std::panic::catch_unwind(code).map_err(|payload| extract_panic_message(payload.as_ref()));
+    #[cfg(debug_assertions)]
+    ERROR_CONTEXT_STACK.with(|cell| cell.borrow_mut().pop_function());
+    result
 }
 
 // TODO(bromeon): make call_ctx lazy-evaluated (like error_ctx) everywhere;
@@ -286,7 +357,7 @@ pub fn handle_varcall_panic<F, R>(
     F: FnOnce() -> Result<R, CallError> + std::panic::UnwindSafe,
 {
     let outcome: Result<Result<R, CallError>, String> =
-        handle_panic_with_print(|| call_ctx, code, false);
+        handle_panic(|| format!("{call_ctx}"), code);
 
     let call_error = match outcome {
         // All good.
@@ -315,7 +386,7 @@ pub fn handle_ptrcall_panic<F, R>(call_ctx: &CallContext, code: F)
 where
     F: FnOnce() -> R + std::panic::UnwindSafe,
 {
-    let outcome: Result<R, String> = handle_panic_with_print(|| call_ctx, code, false);
+    let outcome: Result<R, String> = handle_panic(|| format!("{call_ctx}"), code);
 
     let call_error = match outcome {
         // All good.
@@ -344,91 +415,6 @@ fn report_call_error(call_error: CallError, track_globally: bool) -> i32 {
     }
 }
 
-fn handle_panic_with_print<E, F, R, S>(error_context: E, code: F, print: bool) -> Result<R, String>
-where
-    E: FnOnce() -> S,
-    F: FnOnce() -> R + std::panic::UnwindSafe,
-    S: std::fmt::Display,
-{
-    #[cfg(debug_assertions)]
-    let info: Arc<Mutex<Option<GodotPanicInfo>>> = Arc::new(Mutex::new(None));
-
-    // Back up previous hook, set new one.
-    #[cfg(debug_assertions)]
-    let prev_hook = {
-        let info = info.clone();
-        let prev_hook = std::panic::take_hook();
-
-        std::panic::set_hook(Box::new(move |panic_info| {
-            if let Some(location) = panic_info.location() {
-                *info.lock().unwrap() = Some(GodotPanicInfo {
-                    file: location.file().to_string(),
-                    line: location.line(),
-                    //backtrace: Backtrace::capture(),
-                });
-            } else {
-                eprintln!("panic occurred, but can't get location information");
-            }
-        }));
-
-        prev_hook
-    };
-
-    // Run code that should panic, restore hook.
-    let panic = std::panic::catch_unwind(code);
-
-    // Restore the previous panic hook if in Debug mode.
-    #[cfg(debug_assertions)]
-    std::panic::set_hook(prev_hook);
-
-    match panic {
-        Ok(result) => Ok(result),
-        Err(err) => {
-            // Flush, to make sure previous Rust output (e.g. test announcement, or debug prints during app) have been printed
-            // TODO write custom panic handler and move this there, before panic backtrace printing.
-            flush_stdout();
-
-            // Handle panic info only in Debug mode.
-            #[cfg(debug_assertions)]
-            {
-                let msg = extract_panic_message(err);
-                let mut msg = format_panic_message(msg);
-
-                // Try to add location information.
-                if let Ok(guard) = info.lock() {
-                    if let Some(info) = guard.as_ref() {
-                        msg = format!("{}\n  at {}:{}", msg, info.file, info.line);
-                    }
-                }
-
-                if print {
-                    godot_error!(
-                        "Rust function panicked: {}\n  Context: {}",
-                        msg,
-                        error_context()
-                    );
-                    //eprintln!("Backtrace:\n{}", info.backtrace);
-                }
-
-                Err(msg)
-            }
-
-            #[cfg(not(debug_assertions))]
-            {
-                let _ = error_context; // Unused warning.
-                let msg = extract_panic_message(err);
-                let msg = format_panic_message(msg);
-
-                if print {
-                    godot_error!("{msg}");
-                }
-
-                Err(msg)
-            }
-        }
-    }
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 #[cfg(test)]

godot-macros/src/class/data_models/func.rs

@@ -473,7 +473,7 @@ fn make_ptrcall_fn(call_ctx: &TokenStream, wrapped_method: &TokenStream) -> Toke
         ) {
             let call_ctx = #call_ctx;
             let _success = ::godot::private::handle_panic(
-                || &call_ctx,
+                || format!("{call_ctx}"),
                 || #invocation
             );
 

itest/rust/src/framework/runner.rs

@@ -186,7 +186,7 @@ impl IntegrationTests {
                     // could not be caught, causing UB at the Godot FFI boundary (in practice, this will be a defined Godot crash with
                     // stack trace though).
                     godot_error!("GDScript test panicked");
-                    godot::private::extract_panic_message(e);
+                    godot::private::extract_panic_message(&e);
                     TestOutcome::Failed
                 }
             };
@@ -327,7 +327,8 @@ fn print_test_pre(test_case: &str, test_file: String, last_file: &mut Option<Str
     if flush {
         // Flush in GDScript, because its own print may come sooner than Rust prints otherwise.
         // (Strictly speaking, this can also happen from Rust, when Godot prints something. So far, it didn't though...)
-        godot::private::flush_stdout();
+        use std::io::Write;
+        std::io::stdout().flush().expect("flush stdout");
     }
 }
 

itest/rust/src/object_tests/dynamic_call_test.rs

@@ -149,26 +149,10 @@ fn dynamic_call_with_panic() {
     assert_eq!(call_error.class_name(), Some("Object"));
     assert_eq!(call_error.method_name(), "call");
 
-    let appendix = if cfg!(debug_assertions) {
-        let mut path = "itest/rust/src/object_tests/object_test.rs".to_string();
-
-        if cfg!(target_os = "windows") {
-            path = path.replace('/', "\\")
-        }
-
-        // Obtain line number dynamically, avoids tedious maintenance on code reorganization.
-        let line = ObjPayload::get_panic_line();
-
-        format!("\n  at {path}:{line}")
-    } else {
-        String::new()
-    };
-
-    let expected_error_message = format!(
-        "godot-rust function call failed: Object::call(&\"do_panic\")\
+    let expected_error_message = "godot-rust function call failed: Object::call(&\"do_panic\")\
         \n  Source: ObjPayload::do_panic()\
-        \n    Reason: [panic]  do_panic exploded{appendix}"
-    );
+        \n    Reason: do_panic exploded"
+        .to_string();
 
     assert_eq!(call_error.to_string(), expected_error_message);
 

itest/rust/src/object_tests/object_test.rs

@@ -925,11 +925,6 @@ impl ObjPayload {
     fn do_panic(&self) {
         panic!("do_panic exploded");
     }
-
-    // Obtain the line number of the panic!() call above; keep equidistant to do_panic() method.
-    pub fn get_panic_line() -> u32 {
-        line!() - 5
-    }
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------