Skip to content
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

Better error handling for systems #11562

Closed
musjj opened this issue Jan 27, 2024 · 5 comments
Closed

Better error handling for systems #11562

musjj opened this issue Jan 27, 2024 · 5 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@musjj
Copy link
Contributor

musjj commented Jan 27, 2024

What problem does this solve or what need does it fill?

Handling Result<T, E> inside systems currently feels a bit awkward. The few options we have right now:

  • Call .unwrap() everywhere. This feels very unidiomatic in Rust.
  • Write custom log messages everywhere. This feels very boilerplaty for what could've been a single ?.
  • Have your system return Result<T, E>, then .pipe() it into bevy::utils::{warn, error, ...}.

I'm liking the third option the most. But the problem is that the log messages does not include any traceback information whatsoever, such as the name of the system where the error originated from. This makes it very hard to debug.

What solution would you like?

The usability of bevy::utils::{warn, error, ...} should be improved to make it easier to work with.

Or maybe there should be a built-in support for systems that returns Result<T, E>.

@musjj musjj added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 27, 2024
@nicopap
Copy link
Contributor

nicopap commented Jan 27, 2024

Another option is to use bevy_mod_sysfail (disclaimer, I am the author)

@nicopap nicopap added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Jan 27, 2024
@nicopap
Copy link
Contributor

nicopap commented Jan 27, 2024

This has been discussed extensively in the past. I'm surprised you are actually the first to open an issue regarding error handling in bevy systems edit: #10874.

The best you are likely to get for now are the pipe error handling systems and bevy_mod_sysfail.

Regarding the traceback in the pipe error handling systems, there is an open issue: #8638

@musjj
Copy link
Contributor Author

musjj commented Jan 27, 2024

Thanks for the crate! I tried it out and the log messages now displays the module the error originated from, which is way better than the default .pipe(). But it still does not log the actual name of the system. Is there a way to configure this?

@nicopap
Copy link
Contributor

nicopap commented Jan 27, 2024

A future release will automatically add the system name to the log message. It's going to be published early next week, see nicopap/bevy_mod_sysfail#5

@nicopap
Copy link
Contributor

nicopap commented Jan 28, 2024

I'm closing this as a Duplicate of #10874 since it's really just asking for the same thing.

@nicopap nicopap closed this as not planned Won't fix, can't repro, duplicate, stale Jan 28, 2024
@NthTensor NthTensor mentioned this issue Dec 1, 2024
4 tasks
github-merge-queue bot pushed a commit that referenced this issue Dec 5, 2024
# Objective

Error handling in bevy is hard. See for reference
#11562,
#10874 and
#12660. The goal of this PR is
to make it better, by allowing users to optionally return `Result` from
systems as outlined by Cart in
<#14275 (comment)>.

## Solution

This PR introduces a new `ScheuleSystem` type to represent systems that
can be added to schedules. Instances of this type contain either an
infallible `BoxedSystem<(), ()>` or a fallible `BoxedSystem<(),
Result>`. `ScheuleSystem` implements `System<In = (), Out = Result>` and
replaces all uses of `BoxedSystem` in schedules. The async executor now
receives a result after executing a system, which for infallible systems
is always `Ok(())`. Currently it ignores this result, but more useful
error handling could also be implemented.

Aliases for `Error` and `Result` have been added to the `bevy_ecs`
prelude, as well as const `OK` which new users may find more friendly
than `Ok(())`.

## Testing

- Currently there are not actual semantics changes that really require
new tests, but I added a basic one just to make sure we don't break
stuff in the future.
- The behavior of existing systems is totally unchanged, including
logging.
- All of the existing systems tests pass, and I have not noticed
anything strange while playing with the examples

## Showcase

The following minimal example prints "hello world" once, then completes.

```rust
use bevy::prelude::*;

fn main() {
    App::new().add_systems(Update, hello_world_system).run();
}

fn hello_world_system() -> Result {
    println!("hello world");
    Err("string")?;
    println!("goodbye world");
    OK
}
```

## Migration Guide

This change should be pretty much non-breaking, except for users who
have implemented their own custom executors. Those users should use
`ScheduleSystem` in place of `BoxedSystem<(), ()>` and import the
`System` trait where needed. They can choose to do whatever they wish
with the result.

## Current Work

+ [x] Fix tests & doc comments
+ [x] Write more tests
+ [x] Add examples
+ [X] Draft release notes

## Draft Release Notes

As of this release, systems can now return results.

First a bit of background: Bevy has hisotrically expected systems to
return the empty type `()`. While this makes sense in the context of the
ecs, it's at odds with how error handling is typically done in rust:
returning `Result::Error` to indicate failure, and using the
short-circuiting `?` operator to propagate that error up the call stack
to where it can be properly handled. Users of functional languages will
tell you this is called "monadic error handling".

Not being able to return `Results` from systems left bevy users with a
quandry. They could add custom error handling logic to every system, or
manually pipe every system into an error handler, or perhaps sidestep
the issue with some combination of fallible assignents, logging, macros,
and early returns. Often, users would just litter their systems with
unwraps and possible panics.

While any one of these approaches might be fine for a particular user,
each of them has their own drawbacks, and none makes good use of the
language. Serious issues could also arrise when two different crates
used by the same project made different choices about error handling.

Now, by returning results, systems can defer error handling to the
application itself. It looks like this:

```rust
// Previous, handling internally
app.add_systems(my_system)
fn my_system(window: Query<&Window>) {
   let Ok(window) = query.get_single() else {
       return;
   };
   // ... do something to the window here
}

// Previous, handling externally
app.add_systems(my_system.pipe(my_error_handler))
fn my_system(window: Query<&Window>) -> Result<(), impl Error> {
   let window = query.get_single()?;
   // ... do something to the window here
   Ok(())
}

// Previous, panicking
app.add_systems(my_system)
fn my_system(window: Query<&Window>) {
   let window = query.single();
   // ... do something to the window here
}

// Now 
app.add_systems(my_system)
fn my_system(window: Query<&Window>) -> Result {
    let window = query.get_single()?;
    // ... do something to the window here
    Ok(())
}
```

There are currently some limitations. Systems must either return `()` or
`Result<(), Box<dyn Error + Send + Sync + 'static>>`, with no
in-between. Results are also ignored by default, and though implementing
a custom handler is possible, it involves writing your own custom ecs
executor (which is *not* recomended).

Systems should return errors when they cannot perform their normal
behavior. In turn, errors returned to the executor while running the
schedule will (eventually) be treated as unexpected. Users and library
authors should prefer to return errors for anything that disrupts the
normal expected behavior of a system, and should only handle expected
cases internally.

We have big plans for improving error handling further:
+ Allowing users to change the error handling logic of the default
executors.
+ Adding source tracking and optional backtraces to errors.
+ Possibly adding tracing-levels (Error/Warn/Info/Debug/Trace) to
errors.
+ Generally making the default error logging more helpful and
inteligent.
+ Adding monadic system combininators for fallible systems.
+ Possibly removing all panicking variants from our api.

---------

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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

2 participants