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: Stabilize the catch_panic feature #26492

Closed

Conversation

alexcrichton
Copy link
Member

This function has remained in std for quite some time now without modifications,
and it's a core building block for robust FFI, so this commit stabilizes the
signature as-is.

It is possible to relax the Send or 'static bounds in the future
additionally.

Closes #25662

This function has remained in std for quite some time now without modifications,
and it's a core building block for robust FFI, so this commit stabilizes the
signature as-is.

It is possible to relax the `Send` or `'static` bounds in the future
additionally.

Closes rust-lang#25662
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 22, 2015
@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 22, 2015
@alexcrichton
Copy link
Member Author

Although not currently done, I'd like to update the release notes if we decide to merge this, and I'd also be fine backporting this to 1.2, but I don't feel a great sense of urgency to do so.

@Gankra
Copy link
Contributor

Gankra commented Jun 22, 2015

Are there any known problems or drawbacks to this design?
On Jun 21, 2015 9:11 PM, "Alex Crichton" notifications@github.com wrote:

Although not currently done, I'd like to update the release notes if we
decide to merge this, and I'd also be fine backporting this to 1.2, but I
don't feel a great sense of urgency to do so.


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

@alexcrichton
Copy link
Member Author

Are there any known problems or drawbacks to this design?

#25662 points out that TLS can be used to subvert the Send and 'static bounds here, and those two bounds are also somewhat arbitrary and can perhaps be too restrictive. Apart from that, though, I'm unaware of any known issues with the current API.

@dgrunwald
Copy link
Contributor

If a panic occurs while a mutable borrow exists, currently only Drop implementations have to worry about seeing an inconsistent state ("panic safety").

The combination of catch_panic + TLS + RefCell allows using an object even after a panic occurred in a mutating operation.
Does this mean all other Rust code now has to worry about panic safety, too? Or do we require that types like RefCell use poisoning?

In the latter case, the 'static bound is still necessary to prevent mutating objects that remain accessible after the panic was caught. The Send bound seems unnecessary in both cases.

From the point of view of FFI callbacks (the primary consumer of this API): the 'static bound is harmless as the FFI interface is dealing with raw pointers. But the Send bound is annoying, given that raw pointers don't directly implement Send. The easiest fix would be to create my high-level wrapper type around the raw pointer before using catch_panic, but that means I get undefined behavior if the constructor function of the wrapper type panics...

Also, rt::unwind is still documented as "This is not safe to call in a nested fashion.". Is that just outdated documentation, or is it undefined behavior to nest catch_panic calls?

@sfackler
Copy link
Member

@dgrunwald I believe the discussion in #25662 covers that question.

@alexcrichton
Copy link
Member Author

After some more discussion at the work week I think we're going to want to make some more changes here, so I'm going to close this in favor of an upcoming RFC.

@brson brson removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 9, 2015
@alexcrichton alexcrichton deleted the stabilize-catch-panic branch September 3, 2015 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants