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

Projects
None yet
6 participants
@alexcrichton
Member

alexcrichton commented Mar 23, 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.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Mar 23, 2015

Collaborator

r? @brson

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

Collaborator

rust-highfive commented Mar 23, 2015

r? @brson

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

@aturon aturon referenced this pull request Mar 23, 2015

Closed

Stabilization for 1.0-beta #22500

75 of 91 tasks complete

@alexcrichton alexcrichton assigned aturon and unassigned brson Mar 23, 2015

@aturon

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Mar 23, 2015

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Mar 23, 2015

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 23, 2015

Member

@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?

Member

alexcrichton commented Mar 23, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Mar 23, 2015

Member

Skylight also requires this in order to use rust stable.

Member

carllerche commented Mar 23, 2015

Skylight also requires this in order to use rust stable.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Mar 23, 2015

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 23, 2015

Member

@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?

Member

alexcrichton commented Mar 23, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Mar 23, 2015

Member

@brson Good catch. Sorry for rushing this along.

Member

aturon commented Mar 23, 2015

@brson Good catch. Sorry for rushing this along.

@alexcrichton alexcrichton changed the title from std: Reexport std::rt::unwind::try in std::ffi to @alexcrichton std: Reexport std::rt::unwind::try in std::thread Mar 24, 2015

@alexcrichton alexcrichton changed the title from @alexcrichton std: Reexport std::rt::unwind::try in std::thread to std: Reexport std::rt::unwind::try in std::thread Mar 24, 2015

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 24, 2015

Member

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

Member

alexcrichton commented Mar 24, 2015

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

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Mar 24, 2015

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bors

bors Mar 24, 2015

Contributor

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

Contributor

bors commented Mar 24, 2015

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

std: Reexport std::rt::unwind::try in std::thread
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

This comment has been minimized.

Show comment
Hide comment
Member

aturon commented Mar 26, 2015

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 26, 2015

Rollup merge of #23651 - alexcrichton:unwind-try, r=aturon
 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

rollup merge of #23651: alexcrichton/unwind-try
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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexcrichton alexcrichton deleted the alexcrichton:unwind-try branch Mar 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment