Unify and simplify command and system error handling #18351

alice-i-cecile · 2025-03-17T04:57:31Z

Objective

ECS error handling is a lovely flagship feature for Bevy 0.16, all in the name of reducing panics and encouraging better error handling (Our API suggests that panicking should be the default #14275).
Currently though, command and system error handling are completely disjoint and use different mechanisms.
Additionally, there's a number of distinct ways to set the default/fallback/global error handler that have limited value. As far as I can tell, this will be cfg flagged to toggle between dev and production builds in 99.9% of cases, with no real value in more granular settings or helpers.
Fixes Unify system and command error handling #17272

Solution

Standardize error handling on the OnceLock global error mechanisms ironed out in Improved Command Errors #17215
- As discussed there, there are serious performance concerns there, especially for commands
- I also think this is a better fit for the use cases, as it's truly global
Move from SystemErrorContext to a more general purpose ErrorContext, which can handle observers and commands more clearly
Cut the superfluous setter methods on App and SubApp
Rename the limited (and unhelpful) fallible_systems example to error_handling, and add an example of command error handling

Testing

Ran the error_handling example.

Notes for reviewers

Do you see a clear way to allow commands to retain &mut World access in the per-command custom error handlers? IMO that's a key feature here (allowing the ad-hoc creation of custom commands), but I'm not sure how to get there without exploding complexity.
I've removed the feature gate on the default_error_handler: contrary to @cart's opinion in Improved Command Errors #17215 I think that virtually all apps will want to use this. Can you think of a category of app that a) is extremely performance sensitive b) is fine with shipping to production with the panic error handler? If so, I can try to gather performance numbers and/or reintroduce the feature flag. UPDATE: see benches at the end of this message.
~~OnceLock is in std: @bushrat011899 what should we do here?~~
Do you have ideas for more automated tests for this collection of features?

Benchmarks

I checked the impact of the feature flag introduced: benchmarks might show regressions. This bears more investigation. I'm still skeptical that there are users who are well-served by a fast always panicking approach, but I'm going to re-add the feature flag here to avoid stalling this out.

github-actions · 2025-03-17T05:25:45Z

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

bushrat011899 · 2025-03-17T05:31:44Z

Re. OnceLock: I've already got a suitable alternative implemented in bevy_platform_support::sync::OnceLock! Should be a drop-in replacement that'll defer to std when possible and use a spinlock when it can't.

crates/bevy_ecs/src/error/handler.rs

bushrat011899

I think this is a good direction to move error handling towards. There's definitely room for iteration, but this is the most flexible and most approachable form of error handling thus far. For example, it may be nice to have fallible plugins (e.g., Plugin::build could return a Result), etc. in the future.

Some minor typos and a suggestion to use bevy_platform_support::sync::OnceLock for no_std compatibility reasons. I do also raise the idea of making the OnceLock itself private and just having a function to initialize the handler. While I think that's a nicer API, I don't see it as a blocker.

bushrat011899 · 2025-03-17T07:34:37Z

crates/bevy_ecs/src/error/handler.rs

+/// # Warning
+///
+/// As this can *never* be overwritten, library code should never set this value.
+pub static GLOBAL_ERROR_HANDLER: OnceLock<fn(BevyError, ErrorContext)> = OnceLock::new();


I think the fact that this is a OnceLock should be a hidden implementation detail users don't interact with, allowing us to experiment with the implementation without breaking user code needlessly. Perhaps exposing a simple set_error_handler(...) function to go with the default_error_handler() function that is already added?

The other direction we can go is to remove default_error_handler completely. I don't think it adds a ton without the feature flag.

I decided to cut default_error_handler: it doesn't make this more misuse resistant, and end users generally won't be creating custom error handling code so the convenience argument doesn't hold water for me either.

bushrat011899 · 2025-03-17T07:39:51Z

crates/bevy_ecs/src/error/handler.rs

-    pub last_run: Tick,
+/// Context for a [`BevyError`] to aid in debugging.
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum ErrorContext {


Future PR: Experimenting with this context type will be very helpful in the future. For example, systems and observers could include the SystemId, allowing a more rich error handler to have differing behaviour based on the system that failed, but entirely up to the user.

FYI I think that sticking complex app-specific error handling in the global error handler is likely to be a bit of a mess. Instead, users should be trying to handle errors as close to where they're generated as possible, and we should provide them the tools required to do that.

I agree with Alice here: complex conditional logic in the default handler will probably be an anti-pattern. We can expect users to want to have complex error handling for their code but much less precision is possible for engine code or ecosystem code. For code you write yourself, there's probably better ways to handle errors (like piping). What the default handler gives is consistency: you don't have to worry about one ecosystem developer choosing to panic and another choosing to silently fail without logging.

That said, we should still probably let users do it. Providing the system ID can't hurt. As an aside, I'd probably like to see the source-location-tracking exposed here. But all of that is firmly in the category of follow-up.

100% agreed on source location tracking, and no objection to exposing system id etc. Seems handy for tooling! Just slap on some "please don't omni-error handling" advice in the docs :)

examples/ecs/error_handling.rs

Co-authored-by: Zachary Harrold <zac@harrold.com.au>

github-actions · 2025-03-17T18:05:27Z

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

NthTensor

First code review from me in a while, sorry if I'm a bit rusty. I like seeing all the red, and I think what you've ended up on is really simple and clear. I don't really have any faults with the implementation, but I do still have some questions.

Do you see a clear way to allow commands to retain &mut World access...

The issue is not that we don't have world-access when handling errors in commands or systems, right? The issue is observers. Is there a reason we can't go with something like the following:

fn error_handler(error: BevyError, ctx: ErrorContext, world: DeferredWorld)

Commands and systems appear to have &mut World access when they call GLOBAL_ERROR_HANDLER.get_or_init(|| panic), which means it should be safe to transmute into UnsafeWorldCell and DeferredWorld, no? And observers already have a deferred world they can use. I'm guess I am probably missing something, but it seems like this would be a way to preserve access to commands.

I've removed the feature gate on the default_error_handler...

I think this is probably the right call, but we should benchmark as it could have perf implications. Unfortunately I do think we should wait to merge this until we get either positive benchmarks or hear Cart's stance, which is the only reason I'm marking this as changes requested.

OnceLock is in std...

The once_cell crate has no_std support via critical_section. It would also let us swap out the handler at runtime. Perhaps we should consider using it instead, although that can be follow-up IMO.

Do you have ideas for more automated tests for this collection of features?

Honestly, I think we've landed on a pretty simple implementation. There doesn't seem to be much room for logic errors. Performance is the major concern.

NthTensor · 2025-03-18T02:03:18Z

crates/bevy_ecs/src/error/handler.rs

+/// # Warning
+///
+/// As this can *never* be overwritten, library code should never set this value.
+pub static GLOBAL_ERROR_HANDLER: OnceLock<fn(BevyError, ErrorContext)> = OnceLock::new();


Thoughts on hiding this from users and having a App::set_error_handler fn which can be called multiple times (overwriting the previous value), and then which actually sets this in App::start? We could even make this private and expose it only through a get_error_handler() function.

I think this might be closer to what bevy users expect from our API, and would eliminate the problems caused by calling it conflicting handlers or trying to change it while the app is running. Or IDK maybe we want to be able to swap it out at runtime for the editor, in which case a OnceLock isn't the right primitive either.

That won't work for standalone bevy_ecs users: they don't have access to App. We also really want this to be truly global, not set per SubApp.

That said, I am down to make this a bit more opaque to give us flexibility and a nicer API. Let me see about newtyping it.

In that case I think the current is probably fine TBH. The API works, it was just a thought.

It does work, but I don't like it! 😂

We could always provide a wrapper as part of App that hides the less nice ECS implementation from most users. But keep the current thing (or something like it) in place.

IMO there should be one way to set this: no need to complicate things.

alice-i-cecile · 2025-03-18T03:14:51Z

The issue is not that we don't have world-access when handling errors in commands or systems, right? The issue is observers. Is there a reason we can't go with something like the following:

Originally yes. I'm increasingly convinced that allowing &mut World access in the global handler is a bit of a trap though. Without a really good motivating use case I'm worried that providing that will encourage omni-handlers. However, I'd really like it in Commands::queue_handled, so you can respond to failed commands right at the call site nice and easily.

That can easily wait for followup though.

Honestly, I think we've landed on a pretty simple implementation. There doesn't seem to be much room for logic errors. Performance is the major concern.

Valid. I can try and spend some time benching this.

…' into from-the-other-direction

This reverts commit b20dc8e.

alice-i-cecile · 2025-03-18T05:51:29Z

@NthTensor I'm seeing in the ballpark of 10% perf regressions on some of the command benchmarks with the configurable error handling enabled. I'm not 100% convinced that's real, but that's enough to sell me on feature-flagging this again for this PR.

github-actions · 2025-03-18T06:17:09Z

You added a new feature but didn't add a description for it. Please update the root Cargo.toml file.

NthTensor · 2025-03-18T16:46:47Z

crates/bevy_ecs/src/error/handler.rs

+/// 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.


Are we sure we want to #[inline] this instead of marking it #[cold]? It's the performance of the branch that dosn't make this call that we have to worry about.

That was Cart's original design 🤷🏽‍♀️ I think that's reasonable, but I don't feel like running more benches here right now.

Yeah this can happen during the RC

NthTensor

This seems like it's in a good state for merge now, especially given that we can still remove the feature flag during the RC if we change our minds.

I wish cargo's feature overrides were easier so we could justify this being the default. I also wonder if using cold might move the performance needle at all. Either way, I very much want this in for the first RC.

alice-i-cecile · 2025-03-18T17:45:41Z

Merging: let's see if I managed to fix CI finally...

cargo run -p build-templated-pages -- update features

@cart

# Objective - ECS error handling is a lovely flagship feature for Bevy 0.16, all in the name of reducing panics and encouraging better error handling (#14275). - Currently though, command and system error handling are completely disjoint and use different mechanisms. - Additionally, there's a number of distinct ways to set the default/fallback/global error handler that have limited value. As far as I can tell, this will be cfg flagged to toggle between dev and production builds in 99.9% of cases, with no real value in more granular settings or helpers. - Fixes #17272 ## Solution - Standardize error handling on the OnceLock global error mechanisms ironed out in #17215 - As discussed there, there are serious performance concerns there, especially for commands - I also think this is a better fit for the use cases, as it's truly global - Move from `SystemErrorContext` to a more general purpose `ErrorContext`, which can handle observers and commands more clearly - Cut the superfluous setter methods on `App` and `SubApp` - Rename the limited (and unhelpful) `fallible_systems` example to `error_handling`, and add an example of command error handling ## Testing Ran the `error_handling` example. ## Notes for reviewers - Do you see a clear way to allow commands to retain &mut World access in the per-command custom error handlers? IMO that's a key feature here (allowing the ad-hoc creation of custom commands), but I'm not sure how to get there without exploding complexity. - I've removed the feature gate on the default_error_handler: contrary to @cart's opinion in #17215 I think that virtually all apps will want to use this. Can you think of a category of app that a) is extremely performance sensitive b) is fine with shipping to production with the panic error handler? If so, I can try to gather performance numbers and/or reintroduce the feature flag. UPDATE: see benches at the end of this message. - ~~`OnceLock` is in `std`: @bushrat011899 what should we do here?~~ - Do you have ideas for more automated tests for this collection of features? ## Benchmarks I checked the impact of the feature flag introduced: benchmarks might show regressions. This bears more investigation. I'm still skeptical that there are users who are well-served by a fast always panicking approach, but I'm going to re-add the feature flag here to avoid stalling this out. ![image](https://github.com/user-attachments/assets/237f644a-b36d-4332-9b45-76fd5cbff4d0) --------- Co-authored-by: Zachary Harrold <zac@harrold.com.au>

…ia the GLOBAL_ERROR_HANDLER (#18454) # Objective There are two related problems here: 1. Users should be able to change the fallback behavior of *all* ECS-based errors in their application by setting the `GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein. 2. The existing solution (#15500) for customizing this behavior is high on boilerplate, not global and adds a great deal of complexity. The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question: `Single` / `Populated` should silently skip the system, but `Res` should panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day). ## Solution I've removed the existing `ParamWarnPolicy`-based configuration, and wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the various schedule executors to properly plumb through errors . Additionally, I've done a small cleanup pass on the corresponding example. ## Testing I've run the `fallible_params` example, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲 ## Questions for reviewers 1. Currently, failed system param validation will result in endless console spam. Do you want me to implement a solution for warn_once-style debouncing somehow? 2. Currently, the error reporting for failed system param validation is very limited: all we get is that a system param failed validation and the name of the system. Do you want me to implement improved error reporting by bubbling up errors in this PR? 3. There is broad consensus that the default behavior for failed system param validation should be set on a per-system param basis. Would you like me to implement that in this PR? My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart. ## Migration Guide `ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed completely. Failures during system param validation are now handled via the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs for more information. --------- Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>

…ia the GLOBAL_ERROR_HANDLER (#18454) There are two related problems here: 1. Users should be able to change the fallback behavior of *all* ECS-based errors in their application by setting the `GLOBAL_ERROR_HANDLER`. See #18351 for earlier work in this vein. 2. The existing solution (#15500) for customizing this behavior is high on boilerplate, not global and adds a great deal of complexity. The consensus is that the default behavior when a parameter fails validation should be set based on the kind of system parameter in question: `Single` / `Populated` should silently skip the system, but `Res` should panic. Setting this behavior at the system level is a bandaid that makes getting to that ideal behavior more painful, and can mask real failures (if a resource is missing but you've ignored a system to make the Single stop panicking you're going to have a bad day). I've removed the existing `ParamWarnPolicy`-based configuration, and wired up the `GLOBAL_ERROR_HANDLER`/`default_error_handler` to the various schedule executors to properly plumb through errors . Additionally, I've done a small cleanup pass on the corresponding example. I've run the `fallible_params` example, with both the default and a custom global error handler. The former panics (as expected), and the latter spams the error console with warnings 🥲 1. Currently, failed system param validation will result in endless console spam. Do you want me to implement a solution for warn_once-style debouncing somehow? 2. Currently, the error reporting for failed system param validation is very limited: all we get is that a system param failed validation and the name of the system. Do you want me to implement improved error reporting by bubbling up errors in this PR? 3. There is broad consensus that the default behavior for failed system param validation should be set on a per-system param basis. Would you like me to implement that in this PR? My gut instinct is that we absolutely want to solve 2 and 3, but it will be much easier to do that work (and review it) if we split the PRs apart. `ParamWarnPolicy` and the `WithParamWarnPolicy` have been removed completely. Failures during system param validation are now handled via the `GLOBAL_ERROR_HANDLER`: please see the `bevy_ecs::error` module docs for more information. --------- Co-authored-by: MiniaczQ <xnetroidpl@gmail.com>

alice-i-cecile added 14 commits March 16, 2025 20:13

Remove App / Subapp helper methods

0ab0788

Remove per-schedule error handling

38baf8f

Rename fallible_systems example to error_handling

155ab34

Remove feature flag

0894a05

Rename error_handler_default for easier grepping

45f23b7

Move and rename command error-handlers

4926938

Move command error handling traits

bc9a808

Swap to a generic error context

72b501f

Unify error handlers and standardize on fn(BevyError, ErrorContext)

d3243a1

Standardize on GlobalErrorHandler

0b8bf3b

Small improvements for module docs

66396f2

Import OnceLock for code quality reasons

38a4915

Helpful traits for ErrorContext

e5f7832

Demonstrate error handling for commands

39bec0d

alice-i-cecile added this to the 0.16 milestone Mar 17, 2025

alice-i-cecile requested a review from cart March 17, 2025 04:57

alice-i-cecile added 2 commits March 16, 2025 22:01

No bevy_ecs imports. Bad!

87f586e

clippy::match_same_arms

d52b347

bushrat011899 reviewed Mar 17, 2025

View reviewed changes

crates/bevy_ecs/src/error/handler.rs Outdated Show resolved Hide resolved

bushrat011899 approved these changes Mar 17, 2025

View reviewed changes

alice-i-cecile and others added 2 commits March 17, 2025 13:43

OnceLock from bevy_platform_support

09f32d7

Co-authored-by: Zachary Harrold <zac@harrold.com.au>

Missing whitespace in doc comment

a437f80

Co-authored-by: Zachary Harrold <zac@harrold.com.au>

NthTensor suggested changes Mar 18, 2025

View reviewed changes

alice-i-cecile added 5 commits March 17, 2025 20:41

More doc test fixes

e12b996

Merge remote-tracking branch 'alice-i-cecile/from-the-other-direction…

8f030d6

…' into from-the-other-direction

Remove stray mentions of default_error_handler in docs

b20dc8e

Re-add default error handler helper

b987d07

This reverts commit b20dc8e.

Re-add and beef up the feature flag

83f040a

alice-i-cecile requested a review from NthTensor March 18, 2025 05:51

cfg gate the OnceLock import

19b90c2

NthTensor reviewed Mar 18, 2025

View reviewed changes

NthTensor approved these changes Mar 18, 2025

View reviewed changes

alice-i-cecile added 2 commits March 18, 2025 10:43

Ignore doc test that relies on cfg gate :(

6e00af7

Ensure comments in Cargo.toml are consistent with neighbors

fb3ebda

alice-i-cecile enabled auto-merge March 18, 2025 17:45

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 18, 2025

alice-i-cecile added 3 commits March 18, 2025 10:46

Regenerate cargo_features.md

4947b99

cargo run -p build-templated-pages -- update features

Broken doc links

2e7f9d2

Spaces not tabs in doc comments :upside_down:

e59acca

alice-i-cecile added this pull request to the merge queue Mar 18, 2025

Merged via the queue into bevyengine:main with commit 5d0505a Mar 18, 2025
36 checks passed

alice-i-cecile mentioned this pull request Mar 20, 2025

Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER #18454

Merged

SpecificProtagonist mentioned this pull request Apr 11, 2025

Remove configurable_error_handler feature #18801

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify and simplify command and system error handling #18351

Unify and simplify command and system error handling #18351

alice-i-cecile commented Mar 17, 2025 •

edited

Loading

github-actions bot commented Mar 17, 2025

bushrat011899 commented Mar 17, 2025

bushrat011899 left a comment

bushrat011899 Mar 17, 2025

alice-i-cecile Mar 17, 2025

alice-i-cecile Mar 18, 2025

bushrat011899 Mar 17, 2025

alice-i-cecile Mar 18, 2025

NthTensor Mar 18, 2025 •

edited

Loading

alice-i-cecile Mar 18, 2025

github-actions bot commented Mar 17, 2025

NthTensor left a comment •

edited

Loading

NthTensor Mar 18, 2025 •

edited

Loading

alice-i-cecile Mar 18, 2025

NthTensor Mar 18, 2025

alice-i-cecile Mar 18, 2025

NthTensor Mar 18, 2025 •

edited

Loading

alice-i-cecile Mar 18, 2025

alice-i-cecile commented Mar 18, 2025

alice-i-cecile commented Mar 18, 2025

github-actions bot commented Mar 18, 2025

NthTensor Mar 18, 2025

alice-i-cecile Mar 18, 2025

NthTensor Mar 18, 2025

NthTensor left a comment

alice-i-cecile commented Mar 18, 2025

Unify and simplify command and system error handling #18351

Unify and simplify command and system error handling #18351

Conversation

alice-i-cecile commented Mar 17, 2025 • edited Loading

Objective

Solution

Testing

Notes for reviewers

Benchmarks

github-actions bot commented Mar 17, 2025

bushrat011899 commented Mar 17, 2025

bushrat011899 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2025

NthTensor left a comment • edited Loading

Choose a reason for hiding this comment

NthTensor Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile commented Mar 18, 2025

alice-i-cecile commented Mar 18, 2025

github-actions bot commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthTensor left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Mar 18, 2025

alice-i-cecile commented Mar 17, 2025 •

edited

Loading

NthTensor Mar 18, 2025 •

edited

Loading

NthTensor left a comment •

edited

Loading

NthTensor Mar 18, 2025 •

edited

Loading

NthTensor Mar 18, 2025 •

edited

Loading