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

Should types like AtomicWaker and AbortHandle be declared UnwindSafe? #2211

Open
najamelan opened this issue Sep 9, 2020 · 3 comments
Open
Labels
A-future Area: futures::future A-task Area: futures::task C-feature-request

Comments

@najamelan
Copy link
Contributor

najamelan commented Sep 9, 2020

I run into issues with a type I would like to be UnwindSafe. It contains an AbortHandle which contains a AtomicWaker and these types aren't unwindsafe because AtomicWaker contains an UnsafeCell.

In the current state of affairs it is up to a user in such situation to go figure out from the futures source code whether these are UnwindSafe but it was forgotten to mark them as such, or whether they are not UnwindSafe but that was not mentioned in the docs.

From a quick look over the code for Abortable I can't immediately see how it could be in an invalid state because of a panic in an inopportune moment, but guaranteeing that with certainty for code one hasn't written is quite some work.

So should these types be marked UnwindSafe and has anyone enough understanding of their internals to quickly verify that? I presume there are other types in futures that are concerned as well (eg. channel Senders/Receivers).

@najamelan najamelan changed the title Should types like AtomicWaker and AbortHandle be declared UnwindSafe Should types like AtomicWaker and AbortHandle be declared UnwindSafe? Sep 9, 2020
@taiki-e taiki-e added A-future Area: futures::future A-task Area: futures::task labels Dec 17, 2020
@taiki-e
Copy link
Member

taiki-e commented Apr 18, 2021

Seems AtomicWaker is not actually UnwindSafe. e.g., If the following clone panicked, AtomicWaker will probably not work well.

*self.waker.get() = Some(waker.clone());

see also tokio-rs/tokio#3689

@najamelan
Copy link
Contributor Author

I suppose that depends on whether the clone can panic?

https://doc.rust-lang.org/src/core/task/wake.rs.html#190-192

It's all unsafe, and I have never looked into this code, so I have no idea.

@taiki-e
Copy link
Member

taiki-e commented Apr 23, 2021

clone can cause panic because you can pass a function that can cause panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-future Area: futures::future A-task Area: futures::task C-feature-request
Projects
None yet
Development

No branches or pull requests

2 participants