Merge pull request #1037 from KevinThierauf/master

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

Author: Bromeon

godot-core/src/init/mod.rs

@@ -49,6 +49,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);
+
+        // Without experimental-features enabled, we can only print panics with godot_print! if the panic occurs on the main (Godot) thread.
+        #[cfg(not(feature = "experimental-threads"))]
+        {
+            let main_thread = std::thread::current().id();
+            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;
@@ -68,8 +79,11 @@ pub unsafe fn __gdext_load_library<E: ExtensionLibrary>(
         success as u8
     };
 
-    let ctx = || "error when loading GDExtension library";
-    let is_success = crate::private::handle_panic(ctx, init_code);
+    // Use std::panic::catch_unwind instead of handle_panic: handle_panic uses TLS, which
+    // calls `thread_atexit` on linux, which sets the hot reloading flag on linux.
+    // Using std::panic::catch_unwind avoids this, although we lose out on context information
+    // for debugging.
+    let is_success = std::panic::catch_unwind(init_code);
 
     is_success.unwrap_or(0)
 }

godot-core/src/lib.rs

@@ -34,6 +34,7 @@ pub mod task {}
 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;
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Global variables
@@ -179,11 +179,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>>() {}
 
@@ -220,15 +215,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>() {
@@ -238,18 +225,49 @@ pub fn extract_panic_message(err: Box<dyn std::any::Any + Send>) -> String {
     }
 }
 
-fn format_panic_message(msg: String) -> String {
+pub fn format_panic_message(panic_info: &std::panic::PanicHookInfo) -> String {
+    let mut msg = extract_panic_message(panic_info.payload());
+
+    if let Some(context) = get_gdext_panic_context() {
+        msg = format!("{msg}\nContext: {context}");
+    }
+
+    let prefix = if let Some(location) = panic_info.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<F>(godot_print: F)
+where
+    F: Fn() -> bool + Send + Sync + 'static,
+{
+    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 = format_panic_message(panic_info);
+        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)
@@ -260,19 +278,75 @@ pub(crate) fn has_error_print_level(level: u8) -> bool {
     ERROR_PRINT_LEVEL.load(atomic::Ordering::Relaxed) >= level
 }
 
+/// Internal type used to store context information for debug purposes. Debug context is stored on the thread-local
+/// ERROR_CONTEXT_STACK, which can later be used to retrieve the current context in the event of a panic. This value
+/// probably shouldn't be used directly; use ['get_gdext_panic_context()'](get_gdext_panic_context) instead.
+#[cfg(debug_assertions)]
+struct ScopedFunctionStack {
+    functions: Vec<*const dyn Fn() -> String>,
+}
+
+#[cfg(debug_assertions)]
+impl ScopedFunctionStack {
+    /// # Safety
+    /// Function must be removed (using [`pop_function()`](Self::pop_function)) before lifetime is invalidated.
+    unsafe fn push_function(&mut self, function: &dyn Fn() -> String) {
+        let function = std::ptr::from_ref(function);
+        #[allow(clippy::unnecessary_cast)]
+        let function = function as *const (dyn Fn() -> String + 'static);
+        self.functions.push(function);
+    }
+
+    fn pop_function(&mut self) {
+        self.functions.pop().expect("function stack is empty!");
+    }
+
+    fn get_last(&self) -> Option<String> {
+        self.functions.last().cloned().map(|pointer| {
+            // 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.
+            unsafe { (*pointer)() }
+        })
+    }
+}
+
+#[cfg(debug_assertions)]
+thread_local! {
+    static ERROR_CONTEXT_STACK: RefCell<ScopedFunctionStack> = const {
+        RefCell::new(ScopedFunctionStack { functions: Vec::new() })
+    }
+}
+
+// Value may return `None`, even from panic hook, if called from a 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;
@@ -285,7 +359,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.
@@ -314,7 +388,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.
@@ -343,91 +417,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-core/src/task/async_runtime.rs

@@ -403,7 +403,7 @@ fn poll_future(godot_waker: Arc<GodotWaker>) {
         return;
     };
 
-    let error_context = || "Godot async task failed";
+    let error_context = || "Godot async task failed".to_string();
 
     // If Future::poll() panics, the future is immediately dropped and cannot be accessed again,
     // thus any state that may not have been unwind-safe cannot be observed later.

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

@@ -485,7 +485,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/builtin_tests/containers/callable_test.rs

@@ -382,7 +382,7 @@ impl CallableRefcountTest {
 #[cfg(since_api = "4.2")]
 pub mod custom_callable {
     use super::*;
-    use crate::framework::{assert_eq_self, quick_thread, ThreadCrosser};
+    use crate::framework::{assert_eq_self, quick_thread, suppress_panic_log, ThreadCrosser};
     use godot::builtin::{Dictionary, RustCallable};
     use godot::prelude::Signal;
     use godot::sys;
@@ -596,7 +596,9 @@ pub mod custom_callable {
         let received = Arc::new(AtomicU32::new(0));
         let received_callable = received.clone();
         let callable = Callable::from_local_fn("test", move |_args| {
-            panic!("TEST: {}", received_callable.fetch_add(1, Ordering::SeqCst))
+            suppress_panic_log(|| {
+                panic!("TEST: {}", received_callable.fetch_add(1, Ordering::SeqCst))
+            })
         });
 
         assert_eq!(Variant::nil(), callable.callv(&varray![]));