Custom panic handlers in the standard library

[alexcrichton]

Tracking issue for rust-lang/rfcs#1328

[sfackler]

See rust-lang-nursery/log#67 for one usage example.

[yberreby]

Is there a good reason for the global panic handler to run even if the panic is recovered from?

#![feature(std_panic, recover)]
use std::panic;

fn main() {
    panic::recover(|| {
        panic!("hello");
    }).unwrap_err();
}

http://is.gd/HP4EyO

This prints thread '<main>' panicked at 'hello', <anon>:6. It seems unexpected to me, since the panic was caught, it shouldn't trigger the global handler, should it? IMO, only uncaught panics should trigger the handler and print a message. If a message needs to be printed, it can be, explicitly, at the recover call site.

[sfackler]

There's some talk about this in the RFC PR, but the main reason is that it's very important for the handler to be able to take stack traces.

In addition, the intended use cases for recover are fairly limited, and almost all of them involve rethrowing the panic payload at a different location after catching it. The thread did panic, so it makes sense that the handler would be called IMO.

[shahn]

My biggest usecase for this is #1089, so I'd love a way to suppress the std panic handler, too. I would like to present my users with an error message, set an appropriate exit code, and be assured that desctructors ran without wrapping basically all my return values in Results when an error happens very rarely, is dependant on certain combinations of user input that can't be checked early/not very deep in the call chain.

[sfackler]

@shahn panic::set_handler(|_| ()); should do that for you.

[samphippen]

I'm trying to do something of the form:

let orig_handler = panic::take_handler();
panic::set_handler(|_| ());
<an actual thing>
panic::set_handler(orig_handler);

and getting the error

src/example_group.rs:41:13: 41:31 error: the trait `for<'r, 'r> core::ops::Fn<(&'r std::panicking::PanicInfo<'r>,)>` is not implemented for the type `Box<for<'r, 'r> core::ops::Fn(&'r std::panicking::PanicInfo<'r>) + Send + Sync>` [E0277]
src/example_group.rs:41             panic::set_handler(orig_panic_handler);
                                    ^~~~~~~~~~~~~~~~~~
src/example_group.rs:41:13: 41:31 help: run `rustc --explain E0277` to see a detailed explanation
src/example_group.rs:41:13: 41:31 note: required by `std::panicking::set_handler`
src/example_group.rs:41:13: 41:31 error: the trait `for<'r, 'r> core::ops::FnOnce<(&'r std::panicking::PanicInfo<'r>,)>` is not implemented for the type `Box<for<'r, 'r> core::ops::Fn(&'r std::panicking::PanicInfo<'r>) + Send + Sync>` [E0277]
src/example_group.rs:41             panic::set_handler(orig_panic_handler);
                                    ^~~~~~~~~~~~~~~~~~
src/example_group.rs:41:13: 41:31 help: run `rustc --explain E0277` to see a detailed explanation
src/example_group.rs:41:13: 41:31 help: the following implementations were found:
src/example_group.rs:41:13: 41:31 help:   <Box<alloc::boxed::FnBox<A, Output=R> + 'a> as core::ops::FnOnce<A>>
src/example_group.rs:41:13: 41:31 help:   <Box<alloc::boxed::FnBox<A, Output=R> + Send + 'a> as core::ops::FnOnce<A>>
src/example_group.rs:41:13: 41:31 note: required by `std::panicking::set_handler`
error: aborting due to 2 previous errors
Could not compile `descriptor`.

I tried taking ownership of the value inside the box with

panic::set_handler(*orig_handler);

but this gives the error

src/example_group.rs:41:13: 41:31 error: the trait `core::marker::Sized` is not implemented for the type `for<'r, 'r> core::ops::Fn(&'r std::panicking::PanicInfo<'r>) + Send + Sync` [E0277]
src/example_group.rs:41             panic::set_handler(*orig_panic_handler);
                                    ^~~~~~~~~~~~~~~~~~
src/example_group.rs:41:13: 41:31 help: run `rustc --explain E0277` to see a detailed explanation
src/example_group.rs:41:13: 41:31 note: `for<'r, 'r> core::ops::Fn(&'r std::panicking::PanicInfo<'r>) + Send + Sync` does not have a constant size known at compile-time
src/example_group.rs:41:13: 41:31 note: required by `std::panicking::set_handler`
error: aborting due to previous error
Could not compile `descriptor`.

I feel like it should be easy to set_handler the result from take_handler and I might be missing a trick here. Does anyone have any advice for me? I really like this API and I'd appreciate any help ^_^

[yberreby]

@samphippen This is ugly, but it works.

#![feature(std_panic, panic_handler)]
use std::panic;

fn main() {
    let orig_handler = panic::take_handler();
    panic::set_handler(|_| ());
    // <an actual thing>
    panic::set_handler(move |info| (*orig_handler)(info));
}

http://is.gd/CzXxcb

[samphippen]

thanks!

[samphippen]

Further to this, I think there's a race or bug in the implementation. If this is the wrong place to report this issue please let me know, and I'll delete this comment and put it in the appropriate place. I'm aware this feature is unstable as well, but I figured I'd report some real world feedback anyway. My use case boils down to the following rust program:

#![feature(panic_handler, recover, std_panic)]
use std::thread;
use std::panic::{self, recover};

fn main() {
    let mut build = Vec::new();
    for _ in 0..10 {
        build.push(thread::spawn(|| {
            let orig_handler = panic::take_handler();

            panic::set_handler(|_| ());

            let _res = recover(|| {
                panic!("fail");
            });


            panic::set_handler(move |info| { (*orig_handler)(info) });
        }));
    }

    let results: Vec<_> = build.into_iter().map(|jh| jh.join()).collect();
    let all_passed = results.into_iter().all(|r| r.is_ok());
    println!("{}", all_passed);
}

When I run this, I get the following output:

     Running `target/debug/main`
thread '<unnamed>' panicked at 'fail', src/bin/main.rs:14
thread '<unnamed>' panicked at 'fail', src/bin/main.rs:14
true

(other runs produce more output, or garbled output, but always fewer than 10 total lines)

Given that I'm running 10 threads, I'd expect to either 10 lines of output, or 0, but not two. Is this an incorrect usage of the API, or is this actually a bug I'm seeing?

edit: versions

$ cargo --version
cargo 0.8.0-nightly (2c1426e 2016-01-15)
$ rustc --version
rustc 1.7.0-nightly (2fb0c5ebc 2016-01-14)
$

[SimonSapin]

Having set_handler be a separate call from take_handler is inherently racy. This is a bug in the API design IMO.

[sfackler]

@samphippen The panic handler is a global resource. If multiple threads are messing with it at once without synchronization, you'll see that kind of behavior.

You will want to take a different approach, possibly involving a handler that looks at a thread local which indicates if the panic should be suppressed.

[yberreby]

Now that recover was stabilized, do we still need global, or even thread-local panic handlers? recover with an error branch seems less error prone, immune (I think) to this kind of race condition, and cleaner overall.

[sfackler]

@SimonSapin Those kinds of race conditions still exist if setting and taking were not separate actions. If multiple threads are adjusting a global resource at once without any synchronization at once, you'll see inconsistent behavior. For example, if the panic handlers were a stack, and each handler could choose whether or not to let the next handler run, the ordering of that stack would be inconsistent and so if any handler doesn't unconditionally allow the next handler to run, you'll see inconsistent behavior.

@yberreby recover hasn't yet been stabilized. Anyway, the use case for panic handlers is e.g. "I want to log panic messages with liblog instead of having them dumped to standard output". recover can't really serve that purpose because the handler will run anyway before the recover intercepts the unwind, recover doesn't have contextual information about the panic like the line number, and you don't necessarily have control over all threads in your program to insert the recovers.

[sfackler]

The issue with resetting an old handler is a problem - it's not great to force it to get wrapped in another closure every time. It seems like adjusting set_handler to take an Into<Box<Fn(&PanicInfo)>> would be what you'd want, but it doesn't look like From<T> is implemented for Box<T>. @aturon is that impl prohibited by coherence or something, or could we add it?

EDIT: Ah, it's actually a bit weirder since it's more like impl<F: Fn(&PanicInfo)> From<F> for Box<Fn(&PanicInfo)>.

[yberreby]

@sfackler Oh? I thought it had been. My mistake.

recover can't really serve that purpose because the handler will run anyway before the recover intercepts the unwind, recover doesn't have contextual information about the panic like the line number, and you don't necessarily have control over all threads in your program to insert the recovers.

Couldn't all of this be solved rather easily, especially since recover is still unstable?

• Add a PanicInfo parameter to recover's FnOnce.
• Have just one panic handler for the main thread, which only runs in case of a bubbled up, unhandled, unrecovered panic.

You usually don't want to have dependencies mess with a global resource, but the current approach encourages this behavior. If you don't want caught panics to be silent, you can always re-throw them and have them bubble up to the main thread, can't you?

[sfackler]

PanicInfo wouldn't be a parameter to the closure, but rather part of the Err return value.

"Just one panic handler" sounds like a global resource to me. How would you configure it? It's absolutely true you don't want dependencies messing with the global handler whenever they want, but you absolutely do want it to be possible for dependencies to adjust the panic handler when asked. For example, you may want your log dependency to install a handler that logs panics: https://doc.rust-lang.org/log/log/fn.log_panics.html

EDIT: Ah, and as I mentioned in a previous comment, it is crucial that you be able to capture backtraces at the point that you "deal" with a panic, whether that be in a panic handler or elsewhere.

[yberreby]

@sfackler You're right, it should be part of Err.

"Just one panic handler" is a global resource, like, say, environment variables. However, with this approach, it's less problematic because recover eliminates the need for it in the majority of cases.

Moreover, we already had a global panic handler. There needs to be one to provide logging in case of completely unhandled panic.

It's not strictly needed for it to be writable, but it's convenient for the use-case you described (panic logging), and not having to wrap main in a giant recover closure. If users want to do the latter and get the same behavior, they can.

[sfackler]

From my perspective, there are a couple things here:

First, recover is a thing that most people should not use. It's intended to be used in a very few contexts, primarily preventing a program from unwinding through FFI layers. We explicitly do not want recover to be used in contexts where people would be catching panics as a matter of course. The RecoverSafe bound makes it unergonomic to work with, and that is to some extent intentional.

Here are a couple of examples that the panic handler API is intended to help deal with. Could you go into a bit more detail on how these kinds of things would work with your proposal? I'm not sure I totally understand it.

I'm writing an e.g. webserver, and use log::log_panics to send panic messages to my logger instead of standard error.

I notice some weird panics popping up, and switch to a panic handler that also captures backtraces of the panic source.

I can't track down what's going on just from a backtrace, so I switch to a panic handler that will trigger a process abort for the panic I'm debugging so I can look at the core dump in a debugger.

Note that the latter two of these cases require that the code that looks at the PanicInfo needs to be running before unwinding begins.

[SimonSapin]

@sfackler Yes, synchronization is needed, and I argue that libstd should provide that synchronization by default. The API could take a closure that is given a the previous handler and return the new one, and is called with a lock held internally. Something like:

pub type Handler = Box<Fn(&PanicInfo) + 'static + Sync + Send>;
pub fn set_handler<F>(F) where F: FnOnce(Handler) -> Handler

Looking at the source now, access to the private static mut HANDLER is already protected by a RW lock.

Note that the first thing set_handler currently does with the given unboxed generic Fn(&PanicInfo) is box it, so it’s not a loss to take it boxed in the first place.

[sfackler]

@SimonSapin the issue is that that kind of API doesn't provide the proper sort of synchronization. For example, the race that @samphippen ran into is still present.

Fundamentally, if you have multiple threads adjusting the panic handler concurrently, it seems to me like you need an external, implementation specific ordering to be imposed for the resulting handler to be consistent.

What use cases were you imagining for which an atomic set_handler/take_handler combo was necessary? I think I'd prefer to implement it via an RAII-style API rather than one with nested closures.

[aturon]

@sfackler

It seems like adjusting set_handler to take an Into<Box<Fn(&PanicInfo)>> would be what you'd want, but it doesn't look like From is implemented for Box. @aturon is that impl prohibited by coherence or something, or could we add it?

Nope, should be fine. I don't remember any deliberate decision not to include it.

[alexcrichton]

The libs team discussed this issue during triage yesterday and the decision was to stabilize

[tomaka]

When the crate is compiled with -C panic=abort, is the hook called after a panic and before the abort? Or is it not called at all? Maybe the docs should have a note about that.

[sfackler]

I believe it will still be called, yeah - is that right @alexcrichton? I'll update the docs.

[alexcrichton]

Yes hooks are called regardless of the panic runtime