diff --git a/crates/bevy_ecs/src/error/bevy_error.rs b/crates/bevy_ecs/src/error/bevy_error.rs index 15e77b022d535..e102970f32231 100644 --- a/crates/bevy_ecs/src/error/bevy_error.rs +++ b/crates/bevy_ecs/src/error/bevy_error.rs @@ -86,6 +86,31 @@ impl BevyError { Self::from(error).with_severity(severity) } + /// Constructs a new [`BevyError`] with the given [`Severity`]. + /// + /// Like [`BevyError::new`], but if the `backtrace` cargo feature is enabled + /// it will use the supplied backtrace instead of capturing a new one. + #[cfg(feature = "std")] + pub fn new_with_backtrace( + severity: Severity, + error: E, + backtrace: std::backtrace::Backtrace, + ) -> Self + where + Box: From, + { + #[cfg(not(feature = "backtrace"))] + drop(backtrace); + BevyError { + inner: Box::new(InnerBevyError { + error: error.into(), + severity, + #[cfg(feature = "backtrace")] + backtrace, + }), + } + } + /// Creates a new [`BevyError`] with the [`Severity::Ignore`] severity. /// /// This is a shorthand for [BevyError::new(Severity::Ignore, error)](BevyError::new). @@ -405,9 +430,7 @@ pub fn bevy_error_panic_hook( ) -> impl Fn(&std::panic::PanicHookInfo) { move |info| { if SKIP_NORMAL_BACKTRACE.replace(false) { - if let Some(payload) = info.payload().downcast_ref::<&str>() { - std::println!("{payload}"); - } else if let Some(payload) = info.payload().downcast_ref::() { + if let Some(payload) = info.payload_as_str() { std::println!("{payload}"); } return; diff --git a/crates/bevy_ecs/src/error/handler.rs b/crates/bevy_ecs/src/error/handler.rs index c2ce7a674b1e2..cfd898c3f5705 100644 --- a/crates/bevy_ecs/src/error/handler.rs +++ b/crates/bevy_ecs/src/error/handler.rs @@ -102,6 +102,11 @@ macro_rules! inner { } /// Defines how Bevy reacts to errors. +/// +/// When writing an error handler, if you want to throw a panic, +/// consider setting [`PANIC_ORIGINATES_FROM_ERROR_HANDLER`]. +/// This lets the executor know that a panic doesn't need to be +/// converted back to a [`BevyError`] and passed to the [`FallbackErrorHandler`]. pub type ErrorHandler = fn(BevyError, ErrorContext); /// Fallback error handler to call when an error is not handled otherwise. @@ -124,6 +129,14 @@ impl Default for FallbackErrorHandler { #[deprecated(since = "0.19.0", note = "Renamed to `FallbackErrorHandler`.")] pub type DefaultErrorHandler = FallbackErrorHandler; +#[cfg(feature = "std")] +std::thread_local! { + /// When deliberately throwing a panic in your [`ErrorHandler`], + /// set this to true to indicate to the executor that the panic + /// should not be turned back into a [`BevyError`]. + pub static PANIC_ORIGINATES_FROM_ERROR_HANDLER: core::cell::Cell = const {core::cell::Cell::new(false)}; +} + /// Error handler that defers to an error's [`Severity`]. #[track_caller] #[inline] @@ -143,6 +156,8 @@ pub fn match_severity(err: BevyError, ctx: ErrorContext) { #[track_caller] #[inline] pub fn panic(error: BevyError, ctx: ErrorContext) { + #[cfg(feature = "std")] + PANIC_ORIGINATES_FROM_ERROR_HANDLER.set(true); inner!(panic, error, ctx); } diff --git a/crates/bevy_ecs/src/schedule/executor/mod.rs b/crates/bevy_ecs/src/schedule/executor/mod.rs index b1c363a87e645..90b1d7afce9cb 100644 --- a/crates/bevy_ecs/src/schedule/executor/mod.rs +++ b/crates/bevy_ecs/src/schedule/executor/mod.rs @@ -311,6 +311,17 @@ mod __rust_begin_short_backtrace { // Call `black_box` to prevent this frame from being tail-call optimized away black_box(system.run((), world)) } + + #[inline(never)] + #[cfg(feature = "std")] + pub(super) fn error_handler( + error_handler: crate::error::ErrorHandler, + err: crate::error::BevyError, + err_context: crate::error::ErrorContext, + ) { + error_handler(err, err_context); + black_box(()); + } } #[cfg(test)] diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 197af9df0c078..3e57f3ed3d41e 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -7,18 +7,24 @@ use core::{any::Any, panic::AssertUnwindSafe}; use fixedbitset::FixedBitSet; #[cfg(feature = "std")] use std::eprintln; -use std::sync::{Mutex, MutexGuard}; +use std::{ + backtrace::Backtrace, + sync::{Mutex, MutexGuard}, +}; #[cfg(feature = "trace")] use tracing::{info_span, Span}; use crate::{ - error::{ErrorContext, ErrorHandler, Result}, + error::{ + BevyError, ErrorContext, ErrorHandler, Result, Severity, + PANIC_ORIGINATES_FROM_ERROR_HANDLER, + }, prelude::Resource, schedule::{ is_apply_deferred, ConditionWithAccess, SystemExecutor, SystemSchedule, SystemWithAccess, }, - system::{RunSystemError, ScheduleSystem}, + system::{RunSystemError, ScheduleSystem, System}, world::{unsafe_world_cell::UnsafeWorldCell, World}, }; #[cfg(feature = "hotpatching")] @@ -294,7 +300,7 @@ impl SystemExecutor for MultiThreadedExecutor { if self.apply_final_deferred { // Do one final apply buffers after all systems have completed // Commands should be applied while on the scope's thread, not the executor's thread - let res = apply_deferred(&state.unapplied_systems, systems, world); + let res = apply_deferred(&state.unapplied_systems, systems, world, error_handler); if let Err(payload) = res { let panic_payload = self.panic_payload.get_mut().unwrap(); *panic_payload = Some(payload); @@ -656,28 +662,23 @@ impl ExecutorState { let system_meta = &self.system_task_metadata[system_index]; let task = async move { - let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - // SAFETY: - // - The caller ensures that we have permission to - // access the world data used by the system. - // - `is_exclusive` returned false - unsafe { - if let Err(RunSystemError::Failed(err)) = + let res = handle_errors( + |system| { + // SAFETY: + // - The caller ensures that we have permission to + // access the world data used by the system. + // - `is_exclusive` returned false + unsafe { __rust_begin_short_backtrace::run_unsafe( system, context.environment.world_cell, ) - { - (context.error_handler)( - err, - ErrorContext::System { - name: system.name(), - last_run: system.get_last_run(), - }, - ); } - }; - })); + }, + system, + context.error_handler, + "System panicked", + ); context.system_completed(system_index, res, system); }; @@ -705,7 +706,12 @@ impl ExecutorState { // SAFETY: `can_run` returned true for this system, which means // that no other systems currently have access to the world. let world = unsafe { context.environment.world_cell.world_mut() }; - let res = apply_deferred(&unapplied_systems, context.environment.systems, world); + let res = apply_deferred( + &unapplied_systems, + context.environment.systems, + world, + context.error_handler, + ); context.system_completed(system_index, res, system); }; @@ -715,19 +721,12 @@ impl ExecutorState { // SAFETY: `can_run` returned true for this system, which means // that no other systems currently have access to the world. let world = unsafe { context.environment.world_cell.world_mut() }; - let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - if let Err(RunSystemError::Failed(err)) = - __rust_begin_short_backtrace::run(system, world) - { - (context.error_handler)( - err, - ErrorContext::System { - name: system.name(), - last_run: system.get_last_run(), - }, - ); - } - })); + let res = handle_errors( + |system| __rust_begin_short_backtrace::run(system, world), + system, + context.error_handler, + "Exclusive system panicked", + ); context.system_completed(system_index, res, system); }; @@ -779,24 +778,20 @@ fn apply_deferred( unapplied_systems: &FixedBitSet, systems: &[SyncUnsafeCell], world: &mut World, + error_handler: ErrorHandler, ) -> Result<(), Box> { for system_index in unapplied_systems.ones() { // SAFETY: none of these systems are running, no other references exist let system = &mut unsafe { &mut *systems[system_index].get() }.system; - let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - system.apply_deferred(world); - })); - if let Err(payload) = res { - #[cfg(feature = "std")] - #[expect(clippy::print_stderr, reason = "Allowed behind `std` feature gate.")] - { - eprintln!( - "Encountered a panic when applying buffers for system `{}`!", - system.name() - ); - } - return Err(payload); - } + handle_errors( + |system| { + system.apply_deferred(world); + Ok(()) + }, + system, + error_handler, + "Encountered a panic while applying system buffers", + )?; } Ok(()) } @@ -840,6 +835,50 @@ unsafe fn evaluate_and_fold_conditions( .fold(true, |acc, res| acc && res) } +/// Handle a potential panic or failed system by invoking the error handler +/// and/or returning a panic payload with which to resume unwinding. +fn handle_errors( + f: impl FnOnce(&mut Box>) -> Result<(), RunSystemError>, + system: &mut Box>, + error_handler: ErrorHandler, + error_message: &str, +) -> Result<(), Box> { + PANIC_ORIGINATES_FROM_ERROR_HANDLER.set(false); + let potential_unwind = std::panic::catch_unwind(AssertUnwindSafe(|| f(system))); + match potential_unwind { + // A panic occurred, but it came from an error handler, so pass it on to be rethrown + Err(payload) if PANIC_ORIGINATES_FROM_ERROR_HANDLER.replace(false) => Err(payload), + // Let the error handler handle the panic, passing on any panic it throws + Err(_) => std::panic::catch_unwind(AssertUnwindSafe(|| { + __rust_begin_short_backtrace::error_handler( + error_handler, + BevyError::new_with_backtrace( + Severity::Panic, + error_message, + Backtrace::disabled(), + ), + ErrorContext::System { + name: system.name(), + last_run: system.get_last_run(), + }, + ); + })), + // System returned an error, let the error handler handle it, passing on any panic it throws + Ok(Err(RunSystemError::Failed(err))) => std::panic::catch_unwind(AssertUnwindSafe(|| { + __rust_begin_short_backtrace::error_handler( + error_handler, + err, + ErrorContext::System { + name: system.name(), + last_run: system.get_last_run(), + }, + ); + })), + // Success + _ => Ok(()), + } +} + /// New-typed [`ThreadExecutor`] [`Resource`] that is used to run systems on the main thread #[derive(Resource, Clone)] pub struct MainThreadExecutor(pub Arc>); @@ -859,7 +898,17 @@ impl MainThreadExecutor { #[cfg(test)] mod tests { + use alloc::string::String; + use core::{ + panic::AssertUnwindSafe, + sync::atomic::{AtomicBool, Ordering::Relaxed}, + }; + use std::panic::catch_unwind; + use crate::{ + error::{ + BevyError, ErrorContext, FallbackErrorHandler, PANIC_ORIGINATES_FROM_ERROR_HANDLER, + }, prelude::Resource, schedule::{IntoScheduleConfigs, MultiThreadedExecutor, Schedule}, system::Commands, @@ -899,4 +948,65 @@ mod tests { schedule.add_systems(((|_: Commands| {}), |_: Commands| {}).chain()); schedule.run(&mut world); } + + #[test] + fn panic_to_error() { + let mut world = World::new(); + + let mut schedule_error = Schedule::default(); + schedule_error.set_executor(MultiThreadedExecutor::new()); + schedule_error.add_systems(|| Err(BevyError::ignore(""))); + + let mut schedule_panic = Schedule::default(); + schedule_panic.set_executor(MultiThreadedExecutor::new()); + schedule_panic.add_systems(|| { + panic!("System's panic payload"); + }); + + static HANDLER_CALLED: AtomicBool = AtomicBool::new(false); + fn handle(_: BevyError, ctx: ErrorContext) { + assert!(matches!(ctx, ErrorContext::System { .. })); + HANDLER_CALLED.store(true, Relaxed); + } + world.insert_resource(FallbackErrorHandler(handle)); + + // System error + schedule_error.run(&mut world); + assert!(HANDLER_CALLED.load(Relaxed)); + + // System panic + HANDLER_CALLED.store(false, Relaxed); + schedule_panic.run(&mut world); + assert!(HANDLER_CALLED.load(Relaxed)); + + const PANIC_PAYLOAD: &str = "UwU"; + fn panic(_: BevyError, ctx: ErrorContext) { + assert!(matches!(ctx, ErrorContext::System { .. })); + PANIC_ORIGINATES_FROM_ERROR_HANDLER.set(true); + panic!("{}", PANIC_PAYLOAD); + } + world.insert_resource(FallbackErrorHandler(panic)); + + // System error, handler panic + let result = catch_unwind(AssertUnwindSafe(|| schedule_error.run(&mut world))); + let payload = result.unwrap_err(); + assert_eq!( + payload + .downcast_ref::() + .map(String::as_str) + .unwrap_or_else(|| payload.downcast_ref::<&str>().unwrap()), + PANIC_PAYLOAD + ); + + // System panic, handler panic + let result = catch_unwind(AssertUnwindSafe(|| schedule_panic.run(&mut world))); + let payload = result.unwrap_err(); + assert_eq!( + payload + .downcast_ref::() + .map(String::as_str) + .unwrap_or_else(|| payload.downcast_ref::<&str>().unwrap()), + PANIC_PAYLOAD + ); + } } diff --git a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs index 6be0a4dd13b6b..5f90c1859db79 100644 --- a/crates/bevy_ecs/src/schedule/executor/single_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/single_threaded.rs @@ -6,9 +6,6 @@ use alloc::string::ToString as _; #[cfg(feature = "trace")] use tracing::info_span; -#[cfg(feature = "std")] -use std::eprintln; - use crate::{ error::{ErrorContext, ErrorHandler}, schedule::{is_apply_deferred, ConditionWithAccess, SystemExecutor, SystemSchedule}, @@ -127,7 +124,7 @@ impl SystemExecutor for SingleThreadedExecutor { } if is_apply_deferred(&**system) { - self.apply_deferred(schedule, world); + self.apply_deferred(schedule, world, error_handler); continue; } @@ -146,12 +143,9 @@ impl SystemExecutor for SingleThreadedExecutor { }); #[cfg(feature = "std")] - #[expect(clippy::print_stderr, reason = "Allowed behind `std` feature gate.")] { - if let Err(payload) = std::panic::catch_unwind(f) { - eprintln!("Encountered a panic in system `{}`!", system.name()); - std::panic::resume_unwind(payload); - } + let res = std::panic::catch_unwind(f); + handle_unwind(res, error_handler, &**system, "System panicked"); } #[cfg(not(feature = "std"))] @@ -163,7 +157,7 @@ impl SystemExecutor for SingleThreadedExecutor { } if self.apply_final_deferred { - self.apply_deferred(schedule, world); + self.apply_deferred(schedule, world, error_handler); } self.evaluated_sets.clear(); self.completed_systems.clear(); @@ -187,10 +181,32 @@ impl SingleThreadedExecutor { } } - fn apply_deferred(&mut self, schedule: &mut SystemSchedule, world: &mut World) { + fn apply_deferred( + &mut self, + schedule: &mut SystemSchedule, + world: &mut World, + error_handler: ErrorHandler, + ) { for system_index in self.unapplied_systems.ones() { let system = &mut schedule.systems[system_index].system; - system.apply_deferred(world); + #[cfg(not(feature = "std"))] + { + system.apply_deferred(world); + let _ = error_handler; + } + + #[cfg(feature = "std")] + { + crate::error::PANIC_ORIGINATES_FROM_ERROR_HANDLER.set(false); + let res = + std::panic::catch_unwind(AssertUnwindSafe(|| system.apply_deferred(world))); + handle_unwind( + res, + error_handler, + &**system, + "Encountered a panic while applying system buffers", + ); + } } self.unapplied_systems.clear(); @@ -240,3 +256,32 @@ fn evaluate_and_fold_conditions( }) .fold(true, |acc, res| acc && res) } + +/// Handle a potential panic by invoking the error handler +#[cfg(feature = "std")] +fn handle_unwind( + potential_unwind: Result<(), alloc::boxed::Box>, + error_handler: ErrorHandler, + in_system: &dyn crate::system::System, + error_message: &str, +) { + if let Err(payload) = potential_unwind { + if crate::error::PANIC_ORIGINATES_FROM_ERROR_HANDLER.replace(false) { + std::panic::resume_unwind(payload); + } + + let err = crate::error::BevyError::new_with_backtrace( + crate::error::Severity::Panic, + error_message, + std::backtrace::Backtrace::disabled(), + ); + __rust_begin_short_backtrace::error_handler( + error_handler, + err, + ErrorContext::System { + name: in_system.name(), + last_run: in_system.get_last_run(), + }, + ); + } +}