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

Add option to not capture panics #578

Open
hwittenborn opened this issue Nov 21, 2023 · 6 comments · May be fixed by #579
Open

Add option to not capture panics #578

hwittenborn opened this issue Nov 21, 2023 · 6 comments · May be fixed by #579

Comments

@hwittenborn
Copy link
Member

It appears that when a panic happens anywhere in the Component trait (or any others that function similarly to it) that panics are captured. After the panic occurs, it seems like certain parts of my application (that I haven't seen a pattern to yet) are no longer running, and then my application is left in an unclean state where it can't really do much without breaking.

I'm not sure if this is intended behavior, but I'd really like it if I could at least have an option to disable this panic capturing.

@AaronErhardt
Copy link
Member

The behavior is probably caused by running the component runtime as a future on the glib runtime (through spawn_local). The only way to improve on this would be to add our own unwind catcher and terminate the app from there, if desired by the user.

@hwittenborn
Copy link
Member Author

Are you saying there wouldn't be a way to propagate errors up to a user-defined function? I'm wanting to capture panics in my main() function, does using spawn_local not provide a good way of doing that?

@hwittenborn
Copy link
Member Author

Mentioning my use case to try to give some clarity on what I'm trying to do, but I'd like to capture all panics in my application from main(), and then upload those panics to a service for further inspection. My application was previously written in GTK4/Libadwaita directly and I was able to do such with relative ease, but from what you're saying I'm not sure if you're thinking it's possible.

@AaronErhardt
Copy link
Member

Most async runtimes capture panics in tasks and I think glib does that too. This is done to prevent a single task from terminating the entire app. Unfortunately, this is what you want in your case.

What I was thinking is to add a panic handler to the Component trait that gets called when something in the component panics. From there, you could stop the app and also do whatever is necessary to send crash reports.

@hwittenborn
Copy link
Member Author

I think the cleanest approach would be for nothing to capture panics, and for them to always just propogate all the way back up the stack.

I don't really like the idea of having to add a panic handler to each Component individually. I'd also like to capture all panics, even those that happen to not be inside/related to Component code (i.e. if a bug in Relm4 caused a panic, I'd want to catch that as well).

I'm thinking of filing an issue with the upstream glib crate to see how feasible it would be to add some sort of functionality to disable panic capturing. Is there anything else you'd think might work for a clean implementation though?

@hwittenborn
Copy link
Member Author

Alright, I think I've figured out a pretty good solution to my issue:

As mentioned at gtk-rs/gtk-rs-core#1229 (comment), letting panics go all the way back up to my main() function isn't much of an option since glib has code that isn't unwind-safe.

Since I just needed to capture panics on a global level, I've found this solution to work just fine though:

  • Set panic = "abort" in Cargo.toml, so that panics always immediately shut everything down.
  • Have the following code get placed, which allows for panics to still be captured:
std::panic::set_hook(Box::new(|info| {
    ...
}));

I didn't know std::panic::set_hook could still run on panic::abort, but since it does it looks like that'll solve my issue pretty cleanly.

This should be pretty much solved for me, feel free to close it if there isn't anything else you want looked at with it.

@AaronErhardt AaronErhardt linked a pull request Nov 22, 2023 that will close this issue
4 tasks
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 a pull request may close this issue.

2 participants