Skip to content

Unify system and command error handling #17272

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
alice-i-cecile opened this issue Jan 9, 2025 · 6 comments · Fixed by #18351
Closed

Unify system and command error handling #17272

alice-i-cecile opened this issue Jan 9, 2025 · 6 comments · Fixed by #18351
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished
Milestone

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 9, 2025

But it's weird that this is not unified with the system result handler

Hmm yeah we should think about how to unify these. I think applying the global handler to the system result handler by default makes some sense. I'd prefer that we tackle unification in a separate PR though.

I had some idea that we could be returning these errors to the scheduler through the sync-point system, idk if that's a viable.

Worth considering all of our options. Imo the direct handler approach is nice because it gives us both granularity and nice performance (no need to queue results up, look up which handler to use, etc). I think we should start from there with essentially optimal performance and then compare against the fancier options later.

I don't really think this will be a niche feature though. Don't we anticipate the editor using custom error handling to populate an error console? But that's nothing to do with the implementation

Hmmm yeah thats true. Although thats an implementation we'd own ourselves, and editor integration would likely be behind a cargo feature (so we could abstract that out from a user perspective). I think the majority of users shouldn't need to explicitly think about custom global error handlers.

Originally posted by @cart in #17215 (comment)

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 9, 2025
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jan 9, 2025
@alice-i-cecile
Copy link
Member Author

System error handling:

  • found in bevy_ecs::error module
  • globally configured via DefaultSystemErrorHandler resource, which has App and Subapp helper methods
  • locally configured via system piping
  • handler is of the form fn(BevyError, SystemErrorContext)

Command error handling:

  • found in bevy_ecs::system::commands module
  • globally configured via the GLOBAL_ERROR_HANDLER OnceLock, which needs the configurable_error_handler feature to be enabled
  • locally configured via EntityCommands::queue_handled
  • handler is of the form fn(&mut World, BevyError)

Proposal:

  • centralized in the bevy_ecs::error module
  • globally configured by setting the DefaultErrorHandler resource, with no helper methods (one way to do this please)
  • locally configured via system piping or queue_handled
  • handler is of the form fn(&mut World, BevyError, EcsErrorContext)

@cart
Copy link
Member

cart commented Mar 12, 2025

One consideration here is Observer error handling (which also uses fn(BevyError, SystemErrorContext)), which is executed with only DeferredWorld access.

github-merge-queue bot pushed a commit that referenced this issue Mar 13, 2025
# Objective

While poking at #17272, I
noticed a few small things to clean up.

## Solution

- Improve the docs
- ~~move `SystemErrorContext` out of the `handler.rs` module: it's not
an error handler~~
@NthTensor
Copy link
Contributor

It wouldn't be too terrible to make all error handling use deferred access. I doubt making structural changes to the world after an error will be common practice, and commands would be fine for that.

@alice-i-cecile
Copy link
Member Author

Poking at this, the hard bits appear to be:

  • passing in the default error handler to commands: I think this needs to be completely re-architected
  • allowing for &mut World or DeferredWorld access: there's a lot of different executors, each with their own constraints and signatures

@cart
Copy link
Member

cart commented Mar 13, 2025

passing in the default error handler to commands: I think this needs to be completely re-architected

Before making changes here, please see this context: #17215. (namely (4) and (5))

@alice-i-cecile
Copy link
Member Author

Indeed. I've been thinking about this more and I'm not seeing a good branchless way to modify the commands design. We may need to standardize on the commands design instead.

github-merge-queue bot pushed a commit that referenced this issue Mar 18, 2025
# 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 <[email protected]>
mockersf pushed a commit that referenced this issue Mar 18, 2025
# 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants