Skip to content
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

std: Reexport std::rt::unwind::try in std::thread #23651

Merged
merged 1 commit into from
Mar 28, 2015

Conversation

alexcrichton
Copy link
Member

This commit provides a safe, but unstable interface for the try functionality
of running a closure and determining whether it panicked or not.

There are two primary reasons that this function was previously marked unsafe:

  1. A vanilla version of this function exposes the problem of exception safety by
    allowing a bare try/catch in the language. It is not clear whether this
    concern should be directly tied to unsafe in Rust at the API level. At this
    time, however, the bounds on ffi::try require the closure to be both
    'static and Send (mirroring those of thread::spawn). It may be possible
    to relax the bounds in the future, but for now it's the level of safety that
    we're willing to commit to.
  2. Panicking while panicking will leak resources by not running destructors.
    Because panicking is still controlled by the standard library, safeguards
    remain in place to prevent this from happening.

The new API is now called catch_panic and is marked as #[unstable] for now.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@aturon aturon mentioned this pull request Mar 23, 2015
91 tasks
@alexcrichton alexcrichton assigned aturon and unassigned brson Mar 23, 2015
@aturon
Copy link
Member

aturon commented Mar 23, 2015

@bors: r+ f610ddf

@brson
Copy link
Contributor

brson commented Mar 23, 2015

@bors r- This is a large step to take with no discussion. There has never been a stable 'try' function in Rust.

@alexcrichton
Copy link
Member Author

@brson ah yes sorry, I feel like I often do carry around a little bit too much in my head!

The motivation for this is that today it is impossible to both safely and in a #[stable] fashion call Rust from C. There is no method of preventing an unwinding back into C, which is currently (and probably will continue to be) undefined behavior. The purpose of this commit is make libraries like git2 (which Cargo uses) build on stable Rust.

@aturon and I talked a lot at lunch today about whether this should be safe or not, and if so what it should look like. We ended up concluding that this signature is both a balance between usability and continuing the current trend of preventions against exception safety problems. An example of this trend is how thread::spawn requires Send + 'static to cross thread boundaries and own its entire contents. Another example is thread::scoped only requiring Send, but not allowing you to discover whether a panic happens or not. Put another way, this function could either be implemented as is with rt::unwind::try or thread::spawn(f).join().

Does that help clear things up, do you think that a change like this requires an RFC?

@carllerche
Copy link
Member

Skylight also requires this in order to use rust stable.

@brson
Copy link
Contributor

brson commented Mar 23, 2015

Right now I only have two reservations: that this is in the 'ffi' module - although that's the critical use case, it doesn't otherwise have anything to do with ffi; that it takes the 'try' name, when we already have one 'try' concept, and there is a design for another in the pipelines, and this is the conservative version of try, that could presumably be superseded someday.

@alexcrichton
Copy link
Member Author

@brson do you have suggestions for what you might prefer to see this name or located? I thought the ffi module was a good place to put this because it should only be used in the context of FFI (coming into Rust from another language). I forgot, though, that we may have a try keyword at some point though. Perhaps something like capture?

@aturon
Copy link
Member

aturon commented Mar 23, 2015

@brson Good catch. Sorry for rushing this along.

@alexcrichton alexcrichton changed the title std: Reexport std::rt::unwind::try in std::ffi @alexcrichton std: Reexport std::rt::unwind::try in std::thread Mar 24, 2015
@alexcrichton alexcrichton changed the title @alexcrichton std: Reexport std::rt::unwind::try in std::thread std: Reexport std::rt::unwind::try in std::thread Mar 24, 2015
@alexcrichton
Copy link
Member Author

Ok, I have updated with the API located at std::thread::catch_panic instead of std::ffi::try. It now also has an R return parameter as well as being unstable instead of stable.

re-r? @aturon and @brson

@aturon
Copy link
Member

aturon commented Mar 24, 2015

I am happy with these changes (thanks again @brson for catching these issues).

Note that we will likely want a version without the 'static constraint, which has a few use cases, but will probably want to be marked unsafe and sternly warned against (due to exception safety issues). I have at least one use case in mind for such a function. I imagine adding this as a _unsafe variant.

@bors
Copy link
Contributor

bors commented Mar 24, 2015

☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts.

This commit provides a safe, but unstable interface for the `try` functionality
of running a closure and determining whether it panicked or not.

There are two primary reasons that this function was previously marked `unsafe`:

1. A vanilla version of this function exposes the problem of exception safety by
   allowing a bare try/catch in the language. It is not clear whether this
   concern should be directly tied to `unsafe` in Rust at the API level. At this
   time, however, the bounds on `ffi::try` require the closure to be both
   `'static` and `Send` (mirroring those of `thread::spawn`). It may be possible
   to relax the bounds in the future, but for now it's the level of safety that
   we're willing to commit to.

2. Panicking while panicking will leak resources by not running destructors.
   Because panicking is still controlled by the standard library, safeguards
   remain in place to prevent this from happening.

The new API is now called `catch_panic` and is marked as `#[unstable]` for now.
@aturon
Copy link
Member

aturon commented Mar 26, 2015

@bors: r+ 4c2ddb3

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 26, 2015
 This commit provides a safe, but unstable interface for the `try` functionality
of running a closure and determining whether it panicked or not.

There are two primary reasons that this function was previously marked `unsafe`:

1. A vanilla version of this function exposes the problem of exception safety by
   allowing a bare try/catch in the language. It is not clear whether this
   concern should be directly tied to `unsafe` in Rust at the API level. At this
   time, however, the bounds on `ffi::try` require the closure to be both
   `'static` and `Send` (mirroring those of `thread::spawn`). It may be possible
   to relax the bounds in the future, but for now it's the level of safety that
   we're willing to commit to.

2. Panicking while panicking will leak resources by not running destructors.
   Because panicking is still controlled by the standard library, safeguards
   remain in place to prevent this from happening.

The new API is now called `catch_panic` and is marked as `#[unstable]` for now.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 27, 2015
This commit provides a safe, but unstable interface for the `try` functionality
of running a closure and determining whether it panicked or not.

There are two primary reasons that this function was previously marked `unsafe`:

1. A vanilla version of this function exposes the problem of exception safety by
   allowing a bare try/catch in the language. It is not clear whether this
   concern should be directly tied to `unsafe` in Rust at the API level. At this
   time, however, the bounds on `ffi::try` require the closure to be both
   `'static` and `Send` (mirroring those of `thread::spawn`). It may be possible
   to relax the bounds in the future, but for now it's the level of safety that
   we're willing to commit to.

2. Panicking while panicking will leak resources by not running destructors.
   Because panicking is still controlled by the standard library, safeguards
   remain in place to prevent this from happening.

The new API is now called `catch_panic` and is marked as `#[unstable]` for now.
@bors bors merged commit 4c2ddb3 into rust-lang:master Mar 28, 2015
@alexcrichton alexcrichton deleted the unwind-try branch March 28, 2015 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants