Enumerate types which are !UnwindSafe on purpose, fix the rest

[abonander]

Tokio's API has a number of types that are !UnwindSafe and/or !RefUnwindSafe even though a lot of them are meant to be used concurrently from multiple threads/tasks which inherently will require them to survive unwinding anyway.

Searching the issue tracker and PR history, it appears that this is likely not intentional for most of these types, as patches to add explicit UnwindSafe impls appear to always be accepted:

• sync: make AtomicWaker panic safe #3689
• tokio::net::UdpSocket is not marked as UnwindSafe #4336
• add UnwindSafe impls to UdpSocket #4384
• Mark tokio futures as UnwindSafe #4414
• runtime: mark JoinHandle as UnwindSafe (#4414) #4418

Of course, it is possible to just use AssertUnwindSafe, as has been suggested before, but I don't think this is a tenable solution as it's easy to be overinclusive when it comes to futures (since you can just wrap a whole async {} block with it) and unintentionally assert unwind safety on a type that is !UnwindSafe on purpose, which could lead to logic errors, leaks or UB depending on the implementation of the API that's opting-out.

However, I don't see any documentation in general on what types are intended to be !UnwindSafe and which simply haven't been addressed yet, so I'm opening this issue to enumerate the ones I know of and seek discussion on either fixing them or explicitly leaving them as they are. I don't have time to create an exhaustive list, but I'm hoping that this issue can serve as a nexus for collecting this knowledge to make it easier to find in the future.

tokio::sync module

These types are all meant to be used concurrently, so !UnwindSafe seems unintentional.
I suspect it should be possible to fix most of them in one fell swoop by fixing the underlying primitives.
The guard types should probably also take into account whether the contained type is RefUnwindSafe.

• Barrier
• MappedMutexGuard
• Mutex
• MutexGuard
• Notify
• OnceCell
• OwnedMutexGuard
• OwnedRwLockMappedWriteGuard
• OwnedRwLockReadGuard
• OwnedRwLockWriteGuard
• OwnedSemaphorePermit
• RwLock
• RwLockMappedWriteGuard
• RwLockReadGuard
• RwLockWriteGuard
• Semaphore
• SemaphorePermit

Channels

All channel types appear to be !UnwindSafe as well, even ones like mpsc::Sender which can be used from multiple threads concurrently.

[Darksonn]

I want to note that the standard library argues that its mutex should be UnwindSafe because it has poisoning, and our mutexes doesn't.

[abonander]

The std Mutex and RwLock had poisoning long before UnwindSafe existed, however. The very nature of the types allows their contained values to survive unwinding from other threads.

Given that no other lock API, including Tokio's, has bothered to implement poisoning, I assume the conclusion is that bugs due to broken invariants in the state of a type, left by unwinding, are rare enough that it's not worth the inconvenience.

If that's the case, then why should their handling with catch_unwind be any different?

[llamicron]

Maybe related, but when using gotham I'm unable to use tokio::sync::Mutex to wrap shared state because of this. It prevents me from having async methods in the StateData struct.

[de-vri-es]

Additionally, an unlocked Mutex is UnwindSafe in all cases. It is only MutexGuard and friends that should potentially be !UnwindSafe.

[fzyzcjy]

Hi, is there any updates, especially about RwLock / Mutex's unwind safety, and Runtime's? Thanks!

[Darksonn]

Other than #6189 (that you filed), no.

[fzyzcjy]

@Darksonn Thank you all the same.

As for the Mutex/RwLock, do you suggest we just use AssertUnwindSafe and/or make a PR to make it UnwindSafe, or is forbidden to use it across panic at all?

P.S. My use case is like this: To develop https://github.com/fzyzcjy/flutter_rust_bridge, I need to have catch_unwind because panic should never across FFI boundary. But I want to hold a Mutex/RwLock that can be used in multiple calls, thus it crosses the panic boundary.

[Darksonn]

Unwind safety is just a lint to act as a speedbump when catching panics. 🤷‍♀️ If you are sure that your code is correct even if it panics, then go ahead and use AssertUnwindSafe.

[fzyzcjy]

@Darksonn Thanks for the information! However, I am not sure, because I am not the user but the package developer... I am only sure that the usage scenario is above.

[de-vri-es]

Additionally, an unlocked Mutex is UnwindSafe in all cases. It is only MutexGuard and friends that should potentially be !UnwindSafe.

Actually, this isn't true. You can still lock the mutex inside your closure and then panic while holding the lock. The MutexGuard wont be captured from the outer scope but it's still leaving your data in a potentially inconsistent state.

I would guess that UnwindSafe is fine for Mutex, but RefUnwindSafe not.