core: Add WIP panic handler code by AaronErhardt · Pull Request #579 · Relm4/Relm4 · GitHub
Skip to content

core: Add WIP panic handler code - #579

Open
AaronErhardt wants to merge 1 commit into
mainfrom
panic-handler
Open

AaronErhardt wants to merge 1 commit into
mainfrom
panic-handler

Conversation

@AaronErhardt

Copy link
Copy Markdown
Member

Summary

Fixes #578
@hwittenborn

Checklist

  • cargo fmt
  • cargo clippy
  • cargo test
  • updated CHANGES.md

@hwittenborn

Copy link
Copy Markdown
Member

@AaronErhardt

Copy link
Copy Markdown
Member Author

With the approach in this PR you can specify individual behavior for each component. You can choose what should happen and whether the entire app should be aborted or not. Also, it's just a method on the Component trait instead of the panic hook, which most Rust users don't even know about.

@hwittenborn

Copy link
Copy Markdown
Member

You can choose what should happen and whether the entire app should be aborted or not.

That was leading me to think about when you would ever not want the entire app to be aborted (or at least start unwinding, which isn't a current possibility). When a component panics you can't really use it much anymore, and there doesn't seem to be a good way to handle that kind of state from parent components.

I feel like the lack of handling in that regard from parent components would just make things bound to break, as components don't expect that their child components are just going to panic. I know at least in the app that I'm working on that I'm not expecting things to work at all once a panic occurs, and the pre-Relm4 code where panics weren't captured behaved in a similar manner.

@AaronErhardt

Copy link
Copy Markdown
Member Author

Yes, in most cases you want to stop the entire app, so that's the default behavior introduced by this PR. You can still return false from the handle_panic() method if you think you can recover from the panic.

@hwittenborn

Copy link
Copy Markdown
Member

I'm curious if there's ever really a case where you'd want to recover from the panic though @AaronErhardt. I know just imagining about it that there could be a possibility for it, but I can't really think of a concrete use case where you would want to do such (at least on a singular component level like in here).

@AaronErhardt

Copy link
Copy Markdown
Member Author

Let's say you use some crate which has a bad API that panics instead of returning an error (plotters would be a prominent example). You could a worker or component that manages the export of the plot to a file and if a panics happens you can catch it and continue the execution.

Or you want to execute some additional code, for example for recording the application state and only then you want to actually shut down the app.

@hwittenborn

Copy link
Copy Markdown
Member

I'm still a bit questionable on how needed this should be, I feel like there isn't really a case where it provides much use.

You could a worker or component that manages the export of the plot to a file and if a panics happens you can catch it and continue the execution.

I feel like this kind of stuff should be handled in application code itself, e.g. via the use of catch_unwind followed by sending an output message to the component's parent about the error. Something like this dummy code is what I'm imagining would be the best way to tackle these kind of issues:

enum AppOutput {
    ...
    PanicErr(Err)
}

struct AppModel;

impl Component for AppModel {
    ...

    fn update(...) {
        if let Err(err) = catch_unwind(|| { ... }) {
        sender.output(AppOutput::PanicErr(err));
        }
    }

And then any panics outside of that case should just crash the app in my opinion. I can't really see a useful case outside of that.

@AaronErhardt

Copy link
Copy Markdown
Member Author

I think you're right, catching a panic should be handled by the user manually. I've updated the PR now accordingly and extended the functionality to factories.

@hwittenborn hwittenborn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the late review, I've been quite busy recently :/, things should start to get back to normal now though. I left some comments on things I noticed; I haven't looked over everything in this PR yet, but it's mostly functioning pretty much exactly like what I originally wanted.

Comment thread examples/factory_async.rs
self.value = self.value.wrapping_sub(1);
}
}
panic!()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does panic behavior need to be demonstrated with this example? Shouldn't the panic_handling example cover everything just fine?

Comment thread relm4/src/lib.rs
result = handle => match result {
Ok(result) => result,
Err(_) => {
println!("A task panicked, shutting down the application");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does a custom message need to be printed? The panic message gets shown anyway, so I feel like this just adds unnecessary clutter in the program's output.

It also looks like the println! message doesn't get shown when the panic behavior is set to abort instead of unwind in Cargo.toml anyway. I'm still able to handle panics in my application on a global level by running std::panic::set_hook before my application starts, but simply aborting when that behavior isn't set seems like a good default here.

@hwittenborn

hwittenborn commented Jun 2, 2024 •

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to not capture panics

2 participants