Add thread-local custom panic handlers to customize the behavior of thread panics

[yberreby]

Rendered

EDIT: heavily modified since the first commits!
EDIT 2 (3rd of July): provided implementation details and PoC (see #1100 (comment))

[nagisa]

Related: #1089

I’m strongly in favour of this. Here’s a few more motivational points:

1. Makes Erlang-style just-fail tasks more possible;
2. Silent panic!() needs not to emit code for lines, messages and whatnot reducing the size of panic!() by a considerable amount;
3. Fixes rust-lang/rust#24099, in a sense.

It should be noted, that state of (silent) panic is not entirely undetectable: join on thread returns Err side of Result and main thread exits the process with a certain error code.

[pythonesque]

This sounds great to me.

[abonander]

This doesn't need to be implemented as another rule to panic!() since the macro is already marked as stable. It can be created as another macro, silent_panic!(), which would also be more explicit about its intention, so panic!() can hold the invariant that it always prints an error message.

In the rt::unwind API, it looks like just some slight refactoring in begin_unwind and friends to take Option<Box<Any + Send + 'static>>. The line number and filename are necessary for invoking panic callbacks.

[bluss]

Why change the behavior of plain panic!()?

Panics can carry arbitrary values so in libstd style I guess we could have a PanicBuilder::new().silent(true).message("just giving up").panic() - type API. Only half in jest! Since panics can carry non-string objects that can be a nice way to send the options to the place it is caught.

I'm not sure how feasible this is to implement, but as you know the panic value is wrapped in Box<Any + Send> when carried, so the handler can extract a specific panic options value by downcasting.

Edit: You can totally add the check for a panic options here in the code and silence the panic message.

[lilyball]

FWIW, I don't think making this change should be considered a violation of stability. It wouldn't affect the API, so there's no backwards-compatibility issues as far as compilation goes, and I'm finding it hard to imagine how turning off a message printed to stderr on task panic would break anybody.

That said, I'd still prefer to have to opt-in to silent panics. The idea of passing a panic configuration value to panic!() is clever, but also seems like a weird thing to expose. I'd rather just have something like panic_silent!() (which could internally panic with a value of a known type which is tested for in std::panicking::on_panic()).

[abonander]

@kballard It breaks the invariant that panic!() always prints an error message. We can't assume that people aren't relying on panic!() to print an error message even when not passed any arguments.

[lilyball]

@cybergeek94 Printing a message to stderr is not part of the documentation for panic!(). So it's not something we guarantee, it's just something that we happen to do today.

[bstrie]

So having panic!() at the top level of your code would just end your program with no message whatsoever? I'm not a fan of that idea, it seems like a huge gotcha to cause the behavior of this macro to change so fundamentally depending on the lack of arguments. I would prefer a separate macro if this behavior is desired.

[bstrie]

One more thing: even though we consciously and infamously support "catching" unwinding, that doesn't mean we should be encouraging that use as a normal error-handling solution. A panic at runtime indicates a problem, and if you're really panicking so often that you're clogging up your logfiles then I need to ask what your architecture is like that you feel the need to resort to that approach.

[bluss]

@kballard Panicking with a known value for panic_silent! is of course a reasonable idea. I wanted to point out that panicking with values is already exposed, you can pass an arbitrary value to panic!(...) which is by design.

[Havvy]

Instead of having panic_silent!(), might it make sense to have panic!(panic::silent) or something? Basically add a value that isn't currently accepted to panic!().

[llogiq]

Could changing the output of plain panic!() break compiletest? AFAIK it captures stdout/stderr.

FWIW, I think panic!() should always print a message in debug builds, it could be silenced by default in release builds, and an explicit panic_silently!() (yeah, I would write that out, because it should be uncommon) could be added.

This way we don't lose debugging information on panics, and we can have tight code in release builds.

[mitsuhiko]

I think there is a related problem which makes panic not very useful for messaging failure and that there is nothing it can carry. It's very hard for a panic handler to figure out why exactly something panicked. You can't even set an integer to signal something upwards.

I think there is more that needs doing than just removing an error message.

[yberreby]

This doesn't need to be implemented as another rule to panic!() since the macro is already marked as stable.

Why change the behavior of plain panic!()?

Printing a message to stderr is not part of the documentation for panic!(). So it's not something we guarantee, it's just something that we happen to do today.

FWIW, I think panic!() should always print a message in debug builds, it could be silenced by default in release builds, and an explicit panic_silently!() (yeah, I would write that out, because it should be uncommon) could be added.

This way we don't lose debugging information on panics, and we can have tight code in release builds.

@bluss is right, the logging lies in std::panicking::on_panic(). It would be easier than I expected to remove logging.

From what has been brought up, I think the best solution is as follow:

• Allow replacing the default panic handler instead of simply adding new handlers
• Add a few ready-to-use panic handlers to be registered:
  • debug (the current default) - output the thread name, line number, file name and panic message
  • basic - output the panic message only
  • silent - do not output anything
• By default, either use debug, or use it only in debug builds, and basic in release builds

Here's how it would be used:

use std::thread;

fn main() {
    thread::spawn(|| {
        // use the default panic handler, `debug`, the one we currently have

        some_optional.unwrap();
    }).join();

    thread::spawn(|| {
        // Always use the `silent` handler
        thread::on_panic(thread::handlers::silent);

        // This will never display a message

        // Use cases:
        // - abort thread silently on errors unrecoverable at its level,
        //   but recoverable at the process level, and spawn a new one
        a_third_party_function_that_could_fail();
    }).join();

    thread::spawn(|| {
        // Always use the `basic` handler
        thread::on_panic(thread::handlers::basic);

        // This will simply display whatever is passed to `panic!` on panic,
        // much like println!()

        // Use cases:
        // - abort thread on invalid input with a user-friendly message
        if !is_valid(input) {
            panic!("Invalid input - please try again");
        } else {
            // ...
        }

    }).join();


}

This would undoubtedly offer the most flexibility, without breaking backward compatibility. Additionally, panic!() could output nothing, but only when the basic handler is registered.

To have debug messages on debug builds and bare-bones messages on release builds, users could simply do this:

thread::on_panic(
    if cfg!(debug) {
        thread::handlers::debug
    } else {
        thread::handlers::basic
    }
);

Or this could be the default, but I'm not sure rustc supports debug flags, or whether this would be desirable.

What do you think?

PS: I'm not familiar with the RFC process - should I update the RFC to integrate these changes, even if it implies changing the feature name from silent_panic to panic_handlers or something like that?

[nagisa]

Panics can carry arbitrary values

Or carry none. panic!() carries "explicit panic" even though it wasn’t asked to/was asked to not carry anything.

I'm not sure how feasible this is to implement, but as you know the panic value is wrapped in Box<Any + Send> when carried, so the handler can extract a specific panic options value by downcasting.

Changing that to Box<Option<Any + Send>> is a possiblity, I guess.

Allow replacing the default panic handler instead of simply adding new handlers to it

You already can replace panic handler to your own by not using libstd and providing your own panic_fmt language item.

I'm not familiar with the RFC process - should I update the RFC to integrate these changes, even if it implies changing the feature name from silent_panic to panic_handlers or something like that?

Its your RFC, you may change it anytime until it is accepted. Just note what changes you made and keep commit history.

From both implementation complexity and API standpoint I, personally, prefer original suggestion.

[yberreby]

@nagisa

You already can replace panic handler to your own by not using libstd and providing your own panic_fmt language item.

This is arguably not handy for something as simple as changing what is output to stderr. You'd have to reimplement or copy the unwinding mechanism.

Its your RFC, you may change it anytime until it is accepted. Just note what changes you made and keep commit history.

Glad to know that! I'm working on the second iteration.

From both implementation complexity and API standpoint I, personally, prefer original suggestion.

It surely is simpler, but it's less customizable and flexible. I believe panic!() deserves more.

[yberreby]

I've heavily modified the RFC in the last commit.

[llogiq]

+1

[Diggsey]

+1, although there are a few things that need clarifying:

• What's the signature of "on_panic" - presumably it only allows a 'static callback?
• How does this interact with scoped threads: if the handler returns false, does the parent thread still panic?
• How are handlers chained together, repeated calls to "on_panic"?
• How does this interact with "catch_panic"?
• What is the panic handler allowed to do? Is it safe for it to be a completely custom implementation, or is it possible for handlers to violate memory safety by doing the wrong thing?
• If I wanted to set up a channel to send panic information back to the thread which spawned this one, how would that be done?

[Ericson2314]

-1. What bstrie said on encouraging use of panic. Catching panics should only be done for esoteric FFI reasons.

[yberreby]

@Ericson2314 And yet we are currently allowing panic catching. The standard library accounts for panics (mutex poisoning comes to my mind). A lot of library functions can and do panic. If we are to drop support for clean panicking, we should drop support for panic! itself in release builds, since it's only to be used when debugging or in very early prototypes, right?

We need to be consistent. Either we support panics, or we don't. If we do, we need to do it right.

Unwinding a thread's stack and running destructors has uses, even though I would often prefer a Result-based solution. And as I said, since you cannot prevent exceptional panics in third-party functions, you should be able to handle them properly.

@Diggsey I doubt I'm qualified enough, but your questions alone shed light on potential problems, so it would probably better to leave refactoring of the handler mechanism (which already exists, although it's unstable) for a later RFC, and change this one to a simple logging mode selection, which changes the behavior of the on_panic handler depending on the current panic! logging mode, instead of allowing full customization of its behavior.

I'll update the RFC, thanks for your comments.

[sfackler]

An alternative to a Send+Sync global handler for the Java approach would be a global Send+Sync "handler factory" that produces thread-local handlers which on their own wouldn't need any bounds.

[alexcrichton]

@sfackler yeah as @Diggsey mentioned we run these handlers currently before unwinding, so the double-panic part may come into play.

I'm somewhat leaning more towards only having a global handler as it allows building thread-local handlers on top, so in that sense it seems more flexible. Along those lies I'd want the handler to be Send + Sync so we're basically just allowed to call it concurrently.

[arielb1]

+1 for this. Writing an ugly message to stderr isn't the best thing to do when you have a bug.

[dpc]

+1

My current problem is that I am working on a library using coroutines, that is supposed to handle panics of coroutines it manages. It also implements tests that exercises panic conditions in multithreading cases (so the terminal gets severely trashed).

[llogiq]

@dpc Of course as we know from certain logging frameworks the right thing to do in such cases is to use an MPSC queue + thread to write the messages.

[dpc]

@llogiq : I don't understand.

[llogiq]

Your problem is that multiple threads may write to the console. To avoid trashing the terminal with concurrent writes, you need to synchronize the output. Now there are two ways to do that: Either have the panic output lock the console (which has its own set of problems), or send the stack info to a helper thread that your library spins up to output the stack traces. The panic helper thread then becomes the synchronization point. Of course to send the stack infos, one needs some kind of MPSC queue (e.g. a channel).

[dpc]

I don't want to synchronize anything. I'm not printing anything. It's just panic!() in my tests (which I'm testing) that prints things, that I don't asked it to. :)

[llogiq]

So you want to silence panic!()? Or send some info to a web service? Or put it in an errors.log? What exactly do you want panic!() to do?

[nagisa]

@llogiq I don’t think exact use case is important in this discussion. It is pretty clear already that @dpc would need custom panic handlers – global (which are not impossible at the moment, but pretty painful to do) or thread local – anyway.

[dpc]

Yes. I just wanted to state my usecase, and why I +1 this RFC.

[ticki]

👍 Cool stuff.

[llogiq]

@nagisa It is not clear to me if catch_panic would suffice for @dpc 's use case, that's why I'm asking.

[arielb1]

@llogiq

When logging to stderr you can rely on write(2) atomicity.

[arielb1]

What's the status of this?

[yberreby]

@arielb1 As per #1100 (comment), there are some problems with my last proposal. The idea itself - custom panic handler - seems to have been well received, but the proposed implementation is far from perfect.

Unfortunately, my studies take up a lot of time so I probably won't be able to come up with a new, reasonably good implementation spec in the following months. So unless somebody else does, well, it may take a long time for this to be accepted and implemented.

[ranma42]

cc @ranma42 (I started doing some work on TLS and panicking, I might be able to contribute if I get some spare time)

[Syntaf]

I'm all for this PR. I help develop a terminal graphics library and stderr is not a valid stream we want to be printing exceptions to. We currently have no tools to copy / redirect stderr in rust, this would allow us to properly do so

[nagisa]

I'm all for this PR. I help develop a terminal graphics library and stderr is not a valid stream we want to be printing exceptions to. We currently have no tools to copy / redirect stderr in rust, this would allow us to properly do so

This RFC still wouldn’t allow copying or redirecting stderr per se. It is, however, already possible on at least POSIX systems by using dup2(2).

use std::os::raw::c_int;
extern { fn dup2(fd1: c_int, fd2: c_int) };

fn main(){
    // create a fd
    unsafe { assert!(dup2(your_fd, 2) != -1); }
}

[alexcrichton]

For those interested, @sfackler has posted an alternate RFC to this one which is more centered around global handlers.

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

Note that this entering FCP at the same time as #1328 and the two will likely be decided upon as a unit.

[yberreby]

@alexcrichton I'm pretty sure this RFC can't be merged as-is, and I wouldn't want it to, since the issues @sfackler's pointed out haven't been addressed. Should I close it now?

[alexcrichton]

It's true that the libs team would be unlikely to merge this as-is, but it represents an alternative strategy from #1328 that we're willing to consider. Along those lines we're fine leaving this open as a proxy for the "thread local strategy" vs the "global handler strategy".

[alexcrichton]

The libs team discussed this RFC during triage yesterday and the conclusion was to merge #1328 and close this one. Thanks again for the RFC though @yberreby!