Tracking issue for recover stabilization

[aturon]

The recover function is able, on a single thread, to recover from a panic. There are numerous controversies around this API, including the bounds it applies to its closure, its safety, and its location within std. An open RFC seeks to settle these issues and open the door to stabilization.

cc @alexcrichton

[SimonSapin]

I’m playing with making Python bindings for a Rust library, and I got some segfaults when a Rust panic tried to unwind into the Python interpreter’s call stack. I understand that using catch_panic at the FFI boundary is the right thing to do, but what should I do with an Err result? I can establish conventions so that the other side triggers a Python exception, but I’d like to have some more helpful information in it than "Something bad happened somewhere". At least (a string representation of) the message that was given to panic!, ideally also a back trace of the unwinded stack frames.

catch_panic returns Result<T, Box<Any + Send + 'static>>, but Any is not very helpful here. It implements Debug, but that impl only prints "Any" (literally), which isn’t that helpful.

@wycats, how do you deal with panics in Rust-inside-Ruby?

[abonander]

@SimonSapin Looking at the guts of unwinding, Box<Any + Send + 'static> is always going to either be String (if panic!() had formatting arguments) or &'static str (if panic!() was invoked with just a string literal), so you basically downcast it to one, and if that fails you can probably assume it's the other.

I agree, though, the type could be a lot more informative here.

[SimonSapin]

Yeah, I ended up finding this code: https://github.com/rust-lang/rust/blob/d2047bc97d/src/libstd/panicking.rs#L33-L39 , but it’s not very discoverable. And though String and &'static str are the most common cases, panic!() with a single argument can take any type:

SimonSapin: panic!(4)
playbot: thread '<main>' panicked at 'Box<Any>', <anon>:9
playbot: playpen: application terminated with error code 101

So I wanted to propose changing from this (in std::thread):

type Result<T> = Result<T, Box<Any + Send + 'static>>;

to this:

type Result<T> = Result<T, PanicValue>;
struct PanicValue(Box<Any + Send + 'static>);
impl PanicValue {
    fn as_str(&self) -> Option<&str> {
        match self.0.downcast_ref::<&'static str>() {
            Some(s) => Some(*s),
            None => match self.0.downcast_ref::<String>() {
                Some(s) => Some(&s[..]),
                None => None,
            }
        }
    }
}

… but std::thread::Result is already stable :(

[SimonSapin]

Would this as_str belong as a default method on the Any trait?

[abonander]

Why not have impl Debug for Any (+ Send) test for the value being String or &'static str before printing "Any"? It would add negligible overhead, and I don't think anyone should be relying on the exact value it prints. Right now, it's pretty much useless anyways.

In fact, why not cover all the primitives (except obviously &[T], *const T, *mut T, etc)?

[alexchandel]

Why are we making it easier to use and abuse panics, instead of removing inappropriate ones from the standard library?

[SimonSapin]

@alexchandel Even if you don’t choose to use panics, it’s very hard to be 100% sure that a non-trivial program will not panic ever. (Similar to being 100% sure that it doesn’t have any bug.) And since unwinding through an FFI boundary can be undefined behavior, something like catch_panic is necessary at least to protect against that.

[steveklabnik]

Why are we making it easier to use and abuse panics,

There is a whole "Motivations" section of the related RFC which describes this https://github.com/alexcrichton/rfcs/blob/stabilize-catch-panic/text/0000-stabilize-catch-panic.md#motivation

[diwic]

Naming

I would refrain from using "unwind": It's not currently exposed in the stdlib, and also, it could mean "to relax", which is the opposite of panicking, really :-) And if "catch" is undesirable (I don't remember exactly why) then maybe std::panic::capture could be an option? And std::panic::release to re-throw the panic. E g, consider a herd of panicking animals, which you capture in a cage, move them safely over some boundary, and then release them so they panic wildly again. RecoverSafe then becomes either std::panic::CaptureSafe or possibly just std::panic::Safe.

PanicInfo

The panic handlers added the PanicInfo struct, and recover/propagate has been some kind of parallel process. It would be nice if this API could be a little more consistent, so that recover would actually return a panicinfo somehow, and propagate allowed to take one as a parameter. I'm not exactly sure if this means redesigning PanicInfo, so I'm just going to leave that as a thought for now.

Landing pads for extern fn

I was wondering if we need an abort-on-panic landing pad on extern fns. Even with this interface stablized, the code is going to end up like:

extern fn callback_from_c( /* ... */ ) -> /* ... */ {
    let r = std::panic::capture(|| /* some complicated handling */ );
    if r.is_err() { /* convert to some sort of error */ }
}

...it's going to be hard to guarantee to 100% that the "convert to some sort of error" part is never going to panic. Since we don't want UB, maybe we need to insert an abort-on-panic landing pad as part of all extern fn?

[nikomatsakis]

On Sat, Mar 12, 2016 at 12:13:56PM -0800, wthrowe wrote:

My concern with the current bounds is still less with anything about the API itself than with the fact that this changes the rather strange nature of rustc's OIBIT handling (#27554) from something that was probably rarely encountered in practice to something exposed in a stable API in the standard library.

I think the most prominent use of auto traits (aka OIBIT) is Send
and Sync, not this relatively obscure function.

This is going to make changing the OIBIT semantics without breaking the world really hard, and I don't think enough thought has been put into the current form to make locking the language into it desirable.

I am not sure what changes you are describing (there are none in the
works that I am aware of, just clarifications -- most of which in fact
behave the same as the current implementation), but as I said, I would
expect the real problem for compatibility with any change is going to
be Send and Sync!

[bstrie]

I'm happy to see the progress here, and just as happy that we've managed to smooth over the ergonomics without throwing out the bounds entirely.

Unsurprisingly, I'm +1 to a more descriptive name than "recover". This topic comes up so much in the wild that we're going to be perpetually fielding responses to questions like "why does Rust now have exceptions?" and having self-explanatory naming would make that so much less tedious.

To reiterate:

1. "recover" isn't good because recovery isn't really what's happening here, and we especially don't want people to think that we intend recovery to be what this is used for.
2. "catch" is even worse because of the conflation with exceptions mentioned above. This mechanism is not intended for the same task as an exception handler is, and we overwhelmingly resist the notion that it should be used that way. This isn't as catastrophically bad of a name as it used to be now that the bounds are remaining, but it's still going to make my job much harder (and that includes variants like catch_unwind).
3. Name should contain "unwind" because that's presumably the accepted terminology that we'll be using in manual and guides referring to this feature, as well as in documents that discuss panics in general.

I like @nikomatsakis 's halt_unwind and @alexcrichton 's prevent_unwind out of the options presented so far, and @Amanieu 's AssertUnwindSafe.

[aturon]

@pnkfelix semi-jokingly proposes recoil :)

[glaebhoerl]

what about "intercept"?

[abonander]

Looks like the discussion is still open for a name so I'll throw out a few ideas. This thread's a little long and these are somewhat generic so it's a little too tedious this late at night to Ctrl-F to see if any of them have already been mentioned, so bear with me.

recover->

• guard (like guard_from_unwinding but not intentionally unwieldy)
• barrier (panic::barrier is pretty descriptive, methinks)
• contain (like containing a panicked crowd or herd of animals)
• sequester (if you're feeling pretentious)

propagate->

• Keep it? It's pretty self-explanatory.
• release (I know this one's already been mentioned but it parallels well with contain)
• resume (heavily implies continuation of a previously caught panic, which I think is generally desired)

I kind of agree with the abort landing pads in extern fn, though it should be trivial to disable them so the people who do their homework and use recover (or what-have-you) properly don't have pay for them. I'm thinking a function attribute, like #[recover_safe] so that it's right next to documentation explaining how panics are handled properly. Or maybe, like integer overflow traps, they should only be emitted in debug builds. It could also be a lint that warns whenever possibly fallible operations are undertaken, namely calling into non-extern functions and methods, but only once per extern fn.

[alexcrichton]

So far the front-runner in terms of naming to me is catch_unwind as it invokes both the idea that something is being caught but only unwinds as it won't catch panics-as-aborts. The verb "catch" seems interchangeable to me, so something like guard_unwind also seems fine.

For the propagation function, I'd expect something similar like continue_unwind or resume_unwind which clearly indicates that it's only intended for unwinding and isn't useful in a panics-as-aborts world.

[sfackler]

I like catch_unwind and resume_unwind unwind personally.

It is worth noting, though, that resume_unwind can still be called in an panic-as-aborts context, it'll just abort without running the panic hook. I don't really think that's too big of a deal though.

[huonw]

When considering names, the module path is relevant because functions are typically called via it. Things like panic::catch_unwind seem awkward to me: it's three verbs in a row.

Returning #27719 (comment), particularly its suggestion of introducing a submodule to std::panic, unwind::catch_unwind is bad, but maybe unwind::catch is OK? (Modulo the negative connotations of the catch word, of course, and the possibility that catch will become a keyword.)

[aturon]

Like @sfackler, I prefer catch_unwind and resume_unwind, scoped directly in the panic module, rather than introducing a new submodule.

[BurntSushi]

I think I'm happy as long as unwind is in the name.

[pnkfelix]

I thought the barrier suggestion from @cybergeek94 was good. Especially if we put it into an unwind submodule, yielding unwind::barrier

To me barrier carries the notion that the unwinding will stop here, but there's the hint that you should perhaps trying to "restart it", to keep it going across the boundary introduced by the barrier, if possible, or at least signal that it occurred.

---

Update: unwind::guard seems okay too, but for some reason I prefer unwind::barrier; not sure why.

[e-oz]

My vote against "barrier", sounds too non-related to execution flow.

On Wed, 6 Apr 2016 at 22:32, Felix S Klock II notifications@github.com
wrote:

I thought the barrier suggestion from @cybergeek94
https://github.com/cybergeek94 was good. Especially if we put it into
an unwind submodule, yielding unwind::barrier

To me barrier carries the notion that the unwinding will stop here, but
there's the hint that you should perhaps trying to "restart it", to keep it
going across the boundary introduced by the barrier, if possible, or at
least signal that it occurred.

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#27719 (comment)

[tomaka]

In system programming, barrier usually means "memory barrier", which has nothing to do with unwinding.

[alexcrichton]

The libs team discussed this during triage yesterday and the decision was to stabilize with the following names:

• recover -> catch_unwind
• propagate -> resume_unwind
• AssertRecoverSafe -> AssertUnwindSafe

Thanks again for all the discussion everyone!

[strega-nil]

yay :)