RFC: Stabilize catch_panic

text/0000-stabilize-catch-panic.md

@@ -5,21 +5,21 @@
 
 # Summary
 
-Stabilize `std::thread::catch_panic` after removing the `Send` and `'static`
-bounds from the closure parameter.
+Move `std::thread::catch_panic` to `std::panic::recover` after removing the
+`Send` bound from the closure parameter.
 
 # Motivation
 
 In today's stable Rust it's not possible to catch a panic on the thread that
-caused it. There are a number of situations, however, where catching a panic is
+caused it. There are a number of situations, however, where this is
 either required for correctness or necessary for building a useful abstraction:
 
 * It is currently defined as undefined behavior to have a Rust program panic
   across an FFI boundary. For example if C calls into Rust and Rust panics, then
   this is undefined behavior. Being able to catch a panic will allow writing
-  C apis in Rust that do not risk aborting the process they are embedded into.
+  C APIs in Rust that do not risk aborting the process they are embedded into.
 
-* Abstactions like thread pools want to catch the panics of tasks being run
+* Abstractions like thread pools want to catch the panics of tasks being run
   instead of having the thread torn down (and having to spawn a new thread).
 
 Stabilizing the `catch_panic` function would enable these two use cases, but
@@ -34,8 +34,10 @@ This function will run the closure `f` and if it panics return `Err(Box<Any>)`.
 If the closure doesn't panic it will return `Ok(val)` where `val` is the
 returned value of the closure. The closure, however, is restricted to only close
 over `Send` and `'static` data. These bounds can be overly restrictive, and due
-to thread-local storage they can be subverted, making it unclear what purpose
-they serve. This RFC proposes to remove the bounds as well.
+to thread-local storage [they can be subverted][tls-subvert], making it unclear
+what purpose they serve. This RFC proposes to remove the bounds as well.
+
+[tls-subvert]: https://github.com/rust-lang/rust/issues/25662
 
 Historically Rust has purposefully avoided the foray into the situation of
 catching panics, largely because of a problem typically referred to as
@@ -71,7 +73,7 @@ in the face of exceptions, but languages often have constructs to enable these
 sorts of witnesses. Two primary methods of doing so are something akin to
 finally blocks (code run on a normal or exceptional return) or just catching the
 exception. In both cases code which later runs that has access to the original
-data structure then it will see the broken invariants.
+data structure will see the broken invariants.
 
 Now that we've got a better understanding of how an exception might cause a bug
 (e.g. how code can be "exception unsafe"), let's take a look how we can make
@@ -176,12 +178,13 @@ this RFC.
 
 # Detailed design
 
-At its heart, the change this RFC is proposing is to stabilize
-`std::thread::catch_panic` after removing the `Send` and `'static` bounds from
-the closure parameter, modifying the signature to be:
+At its heart, the change this RFC is proposing is to move
+`std::thread::catch_panic` to a new `std::panic` module and rename the function
+to `catch`. Additionally, the `Send` bound from the closure parameter will be
+removed (`'static` will stay), modifying the signature to be:
 
 ```rust
-fn catch_panic<F: FnOnce() -> R, R>(f: F) -> thread::Result<R>
+fn recover<F: FnOnce() -> R + 'static, R>(f: F) -> thread::Result<R>
 ```
 
 More generally, however, this RFC also claims that this stable function does
@@ -194,29 +197,29 @@ exist in the form of panics. What this RFC is adding, however, is a construct
 via which to catch these exceptions within a thread, bringing the standard
 library closer to the exception support in other languages.
 
-Catching a panic (and especially not having `'static` on the bounds list) makes
-it easier to observe broken invariants of data structures shared across the
-`catch_panic` boundary, which can possibly increase the likelihood of exception
-safety issues arising.
+Catching a panic makes it easier to observe broken invariants of data structures
+shared across the `catch_panic` boundary, which can possibly increase the
+likelihood of exception safety issues arising.
 
 The risk of this step is that catching panics becomes an idiomatic way to deal
 with error-handling, thereby making exception safety much more of a headache
-than it is today. Whereas we intend for the `catch_panic` function to only be
-used where it's absolutely necessary, e.g. for FFI boundaries. How do we ensure
-that `catch_panic` isn't overused?
+than it is today (as it's more likely that a broken invariant is later
+witnessed). The `catch_panic` function is intended to only be used
+where it's absolutely necessary, e.g. for FFI boundaries, but how can it be
+ensured that `catch_panic` isn't overused?
 
-There are two key reasons we don't except `catch_panic` to become idiomatic:
+There are two key reasons `catch_panic` likely won't become idiomatic:
 
-1. We have already established very strong conventions around error handling,
-   and in particular around the use of panic and `Result`, and stabilized usage
-   around them in the standard library. There is little chance these conventions
+1. There are already strong and established conventions around error handling,
+   and in particular around the use of panic and `Result` with stabilized usage
+   of them in the standard library. There is little chance these conventions
    would change overnight.
 
-2. We have long intended to provide an option to treat every use of `panic!` as
-   an abort, which is motivated by portability, compile time, binary size, and a
-   number of other factors. Assuming we take this step, it would be extremely
-   unwise for a library to signal expected errors via panics and rely on
-   consumers using `catch_panic` to handle them.
+2. There has long been a desire to treat every use of `panic!` as an abort
+   which is motivated by portability, compile time, binary size, and a number of
+   other factors. Assuming this step is taken, it would be extremely unwise for
+   a library to signal expected errors via panics and rely on consumers using
+   `catch_panic` to handle them.
 
 For reference, here's a summary of the conventions around `Result` and `panic`,
 which still hold good after this RFC:
@@ -273,16 +276,24 @@ roughly analogous to an opaque "an unexpected error has occurred" message.
 Stabilizing `catch_panic` does little to change the tradeoffs around `Result`
 and `panic` that led to these conventions.
 
-## Why remove the bounds?
+## Why remove `Send`?
+
+One of the primary use cases of `recover` is in an FFI context, where lots
+of `*mut` and `*const` pointers are flying around. These two types aren't
+`Send` by default, so having their values cross the `catch_panic` boundary
+would be highly un-ergonomic (albeit still possible). As a result, this RFC
+proposes removing the `Send` bound from the function.
+
+## Why keep `'static`?
+
+This RFC proposes leaving the `'static` bound on the closure parameter for now.
+There isn't a clearly strong case (such as for `Send`) to remove this parameter
+just yet, and it helps mitigate exception safety issues related to shared
+references across the `recover` boundary.
 
-The main reason to remove the `'static` and `Send` bounds on `catch_panic` is
-that they don't actually enforce anything. Using thread-local storage, it's
-possible to share mutable data across a call to `catch_panic` even if that data
-isn't `'static` or `Send`. And allowing borrowed data, in particular, is helpful
-for thread pools that need to execute closures with borrowed data within them;
-essentially, the worker threads are executing multiple "semantic threads" over
-their lifetime, and the `catch_panic` boundary represents the end of these
-"semantic threads".
+There is conversely also not a clearly strong case for *keeping* this bound, but
+as it's the more conservative route (and backwards compatible to remove) it will
+remain for now.
 
 # Drawbacks