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

The content of the lazy static should require RefUnwindSafe #53

Closed
tomaka opened this issue Oct 13, 2016 · 13 comments
Closed

The content of the lazy static should require RefUnwindSafe #53

tomaka opened this issue Oct 13, 2016 · 13 comments

Comments

@tomaka
Copy link

tomaka commented Oct 13, 2016

It is possible that a panic happens while a lazy static is being modified, and leave the lazy static in an invalid state.

The user could then catch the unwind and continue to use the object in its invalid state.

That's the reason why the UnwindSafe and RefUnwindSafe traits were invented.

@tomaka tomaka changed the title The content of the lazy static should implement RefUnwindSafe The content of the lazy static should require RefUnwindSafe Oct 13, 2016
@tomaka
Copy link
Author

tomaka commented Oct 31, 2016

Related comment: rust-lang/rust#37136 (comment)

@Kimundi
Copy link
Contributor

Kimundi commented Mar 4, 2017

Hi, I'm sincerely sorry for taking so long to respond.

Could you elaborate a bit on which kind of modification is the problem here? I'm not sure if the issue refers to the Once-based initial modification, or a content-dependent one involving other internal mutability in it.

@tomaka
Copy link
Author

tomaka commented Mar 4, 2017

It's about the content of the static having internal mutability.

Imagine you call a method on the static object. This method takes a &self but modifies the content through interior mutability. However unfortunately a panic occurs during the method call, and the internal state of the object becomes invalid.

Later another piece of code accesses the same static object and observes this invalid state.

However keep in mind that the situation about panic safety is a bit blurry in Rust right now. The Send and Sync traits more or less mean "panic safe" already.
I'm not sure if the traits should actually be enforced in addition to Send/Sync, but I thought I'd open an issue anyway.

@Kimundi
Copy link
Contributor

Kimundi commented Mar 4, 2017

Ah, I see.

Am I right in thinking that this issue can in theory happen with a plain old static as well?

If so then I would just mirror what is done in that case - and afaik RefUnwindSafe is not enforced for them.

I'll leave this open until the situation around panic safety becomes more clear. (Also, adding the bound would be a breaking change as well, so I'd rather not experiment needlessly :))

@tomaka
Copy link
Author

tomaka commented Mar 4, 2017

Am I right in thinking that this issue can in theory happen with a plain old static as well?

Well, I don't see how you could modify the content of a non-mut static.

@Kimundi
Copy link
Contributor

Kimundi commented Mar 5, 2017

A plain non-mut static can contain internal mutability types, like for example the atomics from the std lib.

@tomaka
Copy link
Author

tomaka commented Mar 6, 2017

I may not be up to date, but I thought that anything requiring a constructor (eg. a Mutex) couldn't be put in a static.
Atomic types do implement UnwindSafe fwiw.

@Kimundi
Copy link
Contributor

Kimundi commented Mar 7, 2017

It's still a unstable feature, but its allowed now. See here.

The main issue is more the initialization code needed might exceed what is possible within a constant or constexpr, but thats an issue about being able to initialize the thing, not about the unwind semantic if it where constructed in a static.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 27, 2018

My understanding here is that we don't need to worry about the unwind safety during initialization because Once will poison on panic.

For the value itself, we inherit its unwind safety from the standard library:

impl<'a, T: RefUnwindSafe + ?Sized> UnwindSafe for &'a T {}
impl<T: RefUnwindSafe + ?Sized> UnwindSafe for *const T {}
impl<T: RefUnwindSafe + ?Sized> UnwindSafe for *mut T {}

If the value T inside the Lazy contains interior mutability then it won't be unwind safe unless an explicit implementation exists on T. Attempts to use unwind unsafe values in panic::catch_unwind will fail.

So I think the current state of affairs is ok?

@tomaka
Copy link
Author

tomaka commented Sep 27, 2018

Attempts to use unwind unsafe values in panic::catch_unwind will fail.

The problem is not using unwind-unsafe values inside catch_unwind, but after, once you've caught and ignored the panic.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 27, 2018

@tomaka Hmm, you can't get in a situation where you use the value after you've caught and ignored the panic, because you can't catch and ignore the panic in the first place unless the value is unwind safe, right?

@tomaka
Copy link
Author

tomaka commented Sep 27, 2018

Oh, you're saying that you can't access the lazy_static in the first place if it's not UnwindSafe.
I admit that this issue is very old and I don't remember at all my reasoning here.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 28, 2018

That's right, unless you clobber it using AssertUnwindSafe or something. The RefUnwindSafe/UnwindSafe traits seem to have a bit of weird history and since they're fuzzier than Send/Sync they're not so well motivated in our literature...

So I think we're doing the right thing here in lazy_static already with respect to UnwindSafe. Are you happy to close this one now @tomaka?

@tomaka tomaka closed this as completed Sep 29, 2018
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

No branches or pull requests

3 participants