-
Notifications
You must be signed in to change notification settings - Fork 7
Allow suppressing panics. #53
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
base: master
Are you sure you want to change the base?
Conversation
Applies bertptrs/tracing-mutex#53 in advance of an upstream release. Bug: 461821986 Change-Id: I72978485e5d16d90b1ebae59ee89377a792df05a Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1430716 Commit-Queue: Adam Perry <[email protected]> Reviewed-by: Erick Tryzelaar <[email protected]> Fuchsia-Auto-Submit: Adam Perry <[email protected]>
…ual rollout. Applies bertptrs/tracing-mutex#53 in advance of an upstream release. Original-Bug: 461821986 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1430716 Original-Revision: 8b226e6719d5c7f9ca498f1f362c7a98977fbfef GitOrigin-RevId: 3d5e96e2899d66a1d0081ce456a84ad819eabac6 Change-Id: If8b1f7306e802efa676b811f4e7a9409c7d402a5
bertptrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for your contribution. I see your use case and while I would've appreciated an issue before the implementation so we could hash out some details, I think your design is pretty sound.
Aside from some general review comments, I have one concern, whether this wouldn't blow up your program's output if this is a code path that's hit fairly often. Panics have the inherent property that they will terminate the current course of action, avoiding hot loops causing the issue repeatedly. I'm not using this library in practice despite originally thinking I would, so I don't know how that shakes out in practice. Happy to hear your thoughts.
Also, before this PR is merged, could you add a small changelog entry?
examples/suppressed_panics.rs
Outdated
| @@ -0,0 +1,65 @@ | |||
| //! Show what a suppressed crash looks like. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this example? It is simply a carbon copy of the main example, with a single line added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. I agree it's not very nice.
I used it while iterating to manually test the panic suppression, I'll see if I can figure out a real test to leave behind.
src/reporting.rs
Outdated
| // Base message to be reported when cycle is detected | ||
| const BASE_MESSAGE: &str = "Found cycle in mutex dependency graph:"; | ||
|
|
||
| static SHOULD_PANIC: AtomicBool = AtomicBool::new(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this should be a boolean. Printing to stderr, as implemented here, is but one of many "reasonable" things one could do.
I'd probably want to go with an enum instead. Prior art in atomic_enum (not suggesting to use that crate for storing a single enum, that's overkill) suggests that this is not that difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. What do you think about a custom callback API like https://doc.rust-lang.org/std/panic/fn.set_hook.html? That way users would have full control over what happens, and the default callback could be the current panic behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a much better idea than what I had in mind; I figured a large enum with all of the options available but that involves a bunch of conditional compilation for the initial few things I thought of (logging/tracing facade support) came to mind.
Let's go with that. I guess we'd want a Mutex<Box<dyn FnMut + Send>> or Mutex<fn>? The former seems more flexible, but the latter is easier to initialize.
This makes it possible to incrementally adopt tracing-mutex in a library shared by many binaries in the same project without having to fix all potential deadlocks in all binaries at once. The binaries that don't yet have proper lock hygeine can opt-out of real enforcement until their deadlocks are addressed.
Thanks! No worries, happy to discuss on issues beforehand in the future and I'm OK throwing away this code based on feedback. Please don't feel pressured to accept just because the PR exists!
This is definitely an issue. I've actually rolled this change out experimentally at work in some debug builds and yes it creates a lot of spam in the logs when a lock cycle is exercised frequently. Given the severity of a bug that a lock cycle represents it hasn't (yet) been unpopular but I could see a lot of scenarios where it'd be an issue. I think if the crate exposes a custom callback hook it might be easy to leave solutions to this up to users in case there are different approaches for different use cases/contexts. If you're not sure about offering a custom hook API, it might be reasonable to rate limit the messages printed? Or maybe store a map of cycles seen and only print each one once? |
|
I like the idea of the custom hook. As I highlighted in my review comment, I'm not sure of the type for that hook (either With that, I'd just like to ask you to quickly rebase your PR and run a |
This makes it possible to incrementally adopt tracing-mutex in a library shared by many binaries in the same project without having to fix all potential deadlocks in all binaries at once. The binaries that don't yet have proper lock hygeine can opt-out of real enforcement until their deadlocks are addressed.