Skip to content

Remove configurable_error_handler feature #18801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,6 @@ bevy_remote = ["bevy_internal/bevy_remote"]
# Enable input focus subsystem
bevy_input_focus = ["bevy_internal/bevy_input_focus"]

# Use the configurable global error handler as the default error handler.
configurable_error_handler = ["bevy_internal/configurable_error_handler"]

# Enable passthrough loading for SPIR-V shaders (Only supported on Vulkan, shader capabilities and extensions must agree with the platform implementation)
spirv_shader_passthrough = ["bevy_internal/spirv_shader_passthrough"]

Expand Down Expand Up @@ -2215,7 +2212,6 @@ wasm = false
name = "fallible_params"
path = "examples/ecs/fallible_params.rs"
doc-scrape-examples = true
required-features = ["configurable_error_handler"]

[package.metadata.example.fallible_params]
name = "Fallible System Parameters"
Expand All @@ -2227,7 +2223,7 @@ wasm = false
name = "error_handling"
path = "examples/ecs/error_handling.rs"
doc-scrape-examples = true
required-features = ["bevy_mesh_picking_backend", "configurable_error_handler"]
required-features = ["bevy_mesh_picking_backend"]

[package.metadata.example.error_handling]
name = "Error handling"
Expand Down
7 changes: 0 additions & 7 deletions crates/bevy_ecs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,6 @@ bevy_reflect = ["dep:bevy_reflect"]
## Extends reflection support to functions.
reflect_functions = ["bevy_reflect", "bevy_reflect/functions"]

## Use the configurable global error handler as the default error handler.
##
## This is typically used to turn panics from the ECS into loggable errors.
## This may be useful for production builds,
## but can result in a measurable performance impact, especially for commands.
configurable_error_handler = []

## Enables automatic backtrace capturing in BevyError
backtrace = ["std"]

Expand Down
120 changes: 74 additions & 46 deletions crates/bevy_ecs/src/error/handler.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#[cfg(feature = "configurable_error_handler")]
use bevy_platform::sync::OnceLock;
use core::fmt::Display;

use crate::{component::Tick, error::BevyError};
Expand Down Expand Up @@ -77,53 +75,83 @@ impl ErrorContext {
}
}

/// A global error handler. This can be set at startup, as long as it is set before
/// any uses. This should generally be configured _before_ initializing the app.
///
/// This should be set inside of your `main` function, before initializing the Bevy app.
/// The value of this error handler can be accessed using the [`default_error_handler`] function,
/// which calls [`OnceLock::get_or_init`] to get the value.
///
/// **Note:** this is only available when the `configurable_error_handler` feature of `bevy_ecs` (or `bevy`) is enabled!
///
/// # Example
///
/// ```
/// # use bevy_ecs::error::{GLOBAL_ERROR_HANDLER, warn};
/// GLOBAL_ERROR_HANDLER.set(warn).expect("The error handler can only be set once, globally.");
/// // initialize Bevy App here
/// ```
///
/// To use this error handler in your app for custom error handling logic:
///
/// ```rust
/// use bevy_ecs::error::{default_error_handler, GLOBAL_ERROR_HANDLER, BevyError, ErrorContext, panic};
///
/// fn handle_errors(error: BevyError, ctx: ErrorContext) {
/// let error_handler = default_error_handler();
/// error_handler(error, ctx);
/// }
/// ```
///
/// # Warning
///
/// As this can *never* be overwritten, library code should never set this value.
#[cfg(feature = "configurable_error_handler")]
pub static GLOBAL_ERROR_HANDLER: OnceLock<fn(BevyError, ErrorContext)> = OnceLock::new();

/// The default error handler. This defaults to [`panic()`],
/// but if set, the [`GLOBAL_ERROR_HANDLER`] will be used instead, enabling error handler customization.
/// The `configurable_error_handler` feature must be enabled to change this from the panicking default behavior,
/// as there may be runtime overhead.
#[inline]
pub fn default_error_handler() -> fn(BevyError, ErrorContext) {
#[cfg(not(feature = "configurable_error_handler"))]
return panic;
mod global_error_handler {
use super::{panic, BevyError, ErrorContext};
use bevy_platform::sync::atomic::{
AtomicBool, AtomicPtr,
Ordering::{AcqRel, Acquire, Relaxed},
};

/// The default global error handler, cast to a data pointer as Rust doesn't
/// currently have a way to express atomic function pointers.
/// Should we add support for a platform on which function pointers and data pointers
/// have different sizes, the transmutation back will fail to compile. In that case,
/// we can replace the atomic pointer with a regular pointer protected by a `OnceLock`
/// on only those platforms.
/// SAFETY: Only accessible from within this module.
static HANDLER: AtomicPtr<()> = AtomicPtr::new(panic as *mut ());

/// Set the global error handler.
///
/// If used, this should be called [before] any uses of [`default_error_handler`],
/// generally inside your `main` function before initializing the app.
///
/// # Example
///
/// ```
/// # use bevy_ecs::error::{set_global_default_error_handler, warn};
/// set_global_default_error_handler(warn);
/// // initialize Bevy App here
/// ```
///
/// To use this error handler in your app for custom error handling logic:
///
/// ```rust
/// use bevy_ecs::error::{default_error_handler, BevyError, ErrorContext};
///
/// fn handle_errors(error: BevyError, ctx: ErrorContext) {
/// let error_handler = default_error_handler();
/// error_handler(error, ctx);
/// }
/// ```
///
/// # Warning
///
/// As this can *never* be overwritten, library code should never set this value.
///
/// [before]: https://doc.rust-lang.org/nightly/core/sync/atomic/index.html#memory-model-for-atomic-accesses
/// [`default_error_handler`]: super::default_error_handler
pub fn set_global_default_error_handler(handler: fn(BevyError, ErrorContext)) {
// Prevent the handler from being set multiple times.
// We use a separate atomic instead of trying `compare_exchange` on `HANDLER_ADDRESS`
// because Rust doesn't guarantee that function addresses are unique.
static INITIALIZED: AtomicBool = AtomicBool::new(false);
if INITIALIZED
.compare_exchange(false, true, AcqRel, Acquire)
.is_err()
{
panic!("Global error handler set multiple times");
}
HANDLER.store(handler as *mut (), Relaxed);
}

#[cfg(feature = "configurable_error_handler")]
return *GLOBAL_ERROR_HANDLER.get_or_init(|| panic);
/// The default error handler. This defaults to [`panic`],
/// but you can override this behavior via [`set_global_default_error_handler`].
///
/// [`panic`]: super::panic
#[inline]
pub fn default_error_handler() -> fn(BevyError, ErrorContext) {
// The error handler must have been already set from the perspective of this thread,
// otherwise we will panic. It will never be updated after this point.
// We therefore only need a relaxed load.
let ptr = HANDLER.load(Relaxed);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Relaxed operations on a pointer makes me nervous, but I think this is okay for anything but edge cases.

The danger would be that a user wants some run-time configuration for their global error handler, and writes to a global variable before calling set, which then doesn't get synchronized with the load. But writes to a global variable will require their own synchronization, so it's hard to come up with a situation where that leads to a data race, and doing so would require unsafe code elsewhere. (It might also be possible to get into trouble using runtime code generation? But nobody would ever generate an error handler function that way.)

I'd still be inclined to use Acquire here (and Release for the store) to be safer. Acquire loads aren't usually any slower than relaxed ones. In particular, all loads on x86 have Acquire semantics, so they'll compile to the same machine instruction.

// SAFETY: We only ever store valid handler functions.
unsafe { core::mem::transmute(ptr) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rust docs have this to say on the topic of raw-pointer to function-pointer transmutation:

Note that all of this is not portable to platforms where function pointers and data pointers have different sizes.

Is this something we are concerned about? If not, it would probably be good to mention it in or around the safety comment there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment. I haven't added a workaround for those platforms yet because that would need a cfg and not just a branch on a constant, and I don't have a list of those platforms (if they exist yet).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a crate called atomic_fn that does gross int-to-pointer casting but which seems to solve the size issue. I don't think we need to use it, merely mentioning it for posterity in case this becomes an issue later.

}
}

pub use global_error_handler::{default_error_handler, set_global_default_error_handler};

macro_rules! inner {
($call:path, $e:ident, $c:ident) => {
$call!(
Expand Down
12 changes: 5 additions & 7 deletions crates/bevy_ecs/src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
//! All [`BevyError`]s returned by a system, observer or command are handled by an "error handler". By default, the
//! [`panic`] error handler function is used, resulting in a panic with the error message attached.
//!
//! You can change the default behavior by registering a custom error handler.
//! Modify the [`GLOBAL_ERROR_HANDLER`] value to set a custom error handler function for your entire app.
//! You can change the default behavior by registering a custom error handler:
//! Use [`set_global_default_error_handler`]
//! to set a custom error handler function for your entire app.
//! In practice, this is generally feature-flagged: panicking or loudly logging errors in development,
//! and quietly logging or ignoring them in production to avoid crashing the app.
//!
Expand All @@ -33,10 +34,8 @@
//! The [`ErrorContext`] allows you to access additional details relevant to providing
//! context surrounding the error – such as the system's [`name`] – in your error messages.
//!
//! Remember to turn on the `configurable_error_handler` feature to set a global error handler!
//!
//! ```rust, ignore
//! use bevy_ecs::error::{GLOBAL_ERROR_HANDLER, BevyError, ErrorContext};
//! use bevy_ecs::error::{set_global_default_error_handler, BevyError, ErrorContext};
//! use log::trace;
//!
//! fn my_error_handler(error: BevyError, ctx: ErrorContext) {
Expand All @@ -48,8 +47,7 @@
//! }
//!
//! fn main() {
//! // This requires the "configurable_error_handler" feature to be enabled to be in scope.
//! GLOBAL_ERROR_HANDLER.set(my_error_handler).expect("The error handler can only be set once.");
//! set_global_default_error_handler(my_error_handler);
//!
//! // Initialize your Bevy App here
//! }
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ use crate::{
/// which will be passed to an [error handler](crate::error) if the `Result` is an error.
///
/// The [default error handler](crate::error::default_error_handler) panics.
/// It can be configured by setting the `GLOBAL_ERROR_HANDLER`.
/// It can be configured via [`set_global_default_error_handler`](crate::error::set_global_default_error_handler).
///
/// Alternatively, you can customize the error handler for a specific command
/// by calling [`Commands::queue_handled`].
Expand Down Expand Up @@ -1224,7 +1224,7 @@ impl<'w, 's> Commands<'w, 's> {
/// which will be passed to an [error handler](crate::error) if the `Result` is an error.
///
/// The [default error handler](crate::error::default_error_handler) panics.
/// It can be configured by setting the `GLOBAL_ERROR_HANDLER`.
/// It can be configured via [`set_global_default_error_handler`](crate::error::set_global_default_error_handler).
///
/// Alternatively, you can customize the error handler for a specific command
/// by calling [`EntityCommands::queue_handled`].
Expand Down
3 changes: 0 additions & 3 deletions crates/bevy_internal/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,6 @@ custom_cursor = ["bevy_winit/custom_cursor"]
# Experimental support for nodes that are ignored for UI layouting
ghost_nodes = ["bevy_ui/ghost_nodes"]

# Use the configurable global error handler as the default error handler.
configurable_error_handler = ["bevy_ecs/configurable_error_handler"]

# Allows access to the `std` crate. Enabling this feature will prevent compilation
# on `no_std` targets, but provides access to certain additional features on
# supported platforms.
Expand Down
1 change: 0 additions & 1 deletion docs/cargo_features.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ The default feature set enables most of the expected features of a game engine,
|bevy_remote|Enable the Bevy Remote Protocol|
|bevy_ui_debug|Provides a debug overlay for bevy UI|
|bmp|BMP image format support|
|configurable_error_handler|Use the configurable global error handler as the default error handler.|
|critical-section|`critical-section` provides the building blocks for synchronization primitives on all platforms, including `no_std`.|
|dds|DDS compressed texture support|
|debug_glam_assert|Enable assertions in debug builds to check the validity of parameters passed to glam|
Expand Down
9 changes: 2 additions & 7 deletions examples/ecs/error_handling.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
//! Showcases how fallible systems and observers can make use of Rust's powerful result handling
//! syntax.
//!
//! Important note: to set the global error handler, the `configurable_error_handler` feature must be
//! enabled. This feature is disabled by default, as it may introduce runtime overhead, especially for commands.

use bevy::ecs::{
error::{warn, GLOBAL_ERROR_HANDLER},
error::{set_global_default_error_handler, warn},
world::DeferredWorld,
};
use bevy::math::sampling::UniformMeshSampler;
Expand All @@ -22,9 +19,7 @@ fn main() {
// Here we set the global error handler using one of the built-in
// error handlers. Bevy provides built-in handlers for `panic`, `error`, `warn`, `info`,
// `debug`, `trace` and `ignore`.
GLOBAL_ERROR_HANDLER
.set(warn)
.expect("The error handler can only be set once, globally.");
set_global_default_error_handler(warn);

let mut app = App::new();

Expand Down
11 changes: 5 additions & 6 deletions examples/ecs/fallible_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//! from running if their acquiry conditions aren't met.
//!
//! Fallible system parameters include:
//! - [`Res<R>`], [`ResMut<R>`] - Resource has to exist, and the [`GLOBAL_ERROR_HANDLER`] will be called if it doesn't.
//! - [`Res<R>`], [`ResMut<R>`] - Resource has to exist, and the [`default_error_handler`] will be called if it doesn't.
//! - [`Single<D, F>`] - There must be exactly one matching entity, but the system will be silently skipped otherwise.
//! - [`Option<Single<D, F>>`] - There must be zero or one matching entity. The system will be silently skipped if there are more.
//! - [`Populated<D, F>`] - There must be at least one matching entity, but the system will be silently skipped otherwise.
Expand All @@ -18,18 +18,17 @@
//!
//! [`SystemParamValidationError`]: bevy::ecs::system::SystemParamValidationError
//! [`SystemParam::validate_param`]: bevy::ecs::system::SystemParam::validate_param
//! [`default_error_handler`]: bevy::ecs::error::default_error_handler

use bevy::ecs::error::{warn, GLOBAL_ERROR_HANDLER};
use bevy::ecs::error::{set_global_default_error_handler, warn};
use bevy::prelude::*;
use rand::Rng;

fn main() {
// By default, if a parameter fail to be fetched,
// the `GLOBAL_ERROR_HANDLER` will be used to handle the error,
// the `default_error_handler` will be used to handle the error,
// which by default is set to panic.
GLOBAL_ERROR_HANDLER
.set(warn)
.expect("The error handler can only be set once, globally.");
set_global_default_error_handler(warn);

println!();
println!("Press 'A' to add enemy ships and 'R' to remove them.");
Expand Down