Skip to content

Conversation

sobczal2
Copy link
Contributor

Add tests for UnwindSafe and RefUnwindSafe in async_send_sync.rs

Fixes: #7413

Motivation

We want to avoid unintentionally changing UnwindSafe and RefUnwindSafe auto traits in public types.

Solution

Add UnwindSafe and RefUnwindSafe to already checked auto traits in tokio/tests/async_send_sync.rs. For tokio::process::Command those autotraits differ between windows and unix, so it was moved to new os specific sections.

Add tests for UnwindSafe and RefUnwindSafe in async_send_sync.rs

Fixes: tokio-rs#7413
@ADD-SP ADD-SP added the A-tokio Area: The main tokio crate label Jul 27, 2025
@sobczal2
Copy link
Contributor Author

Both UnwindSafe and RefUnwindSafe auto traits differ for configuration using miri and configuration removing parking_lot. I can group them and guard them using cfg for those 3 cases, but I'm not sure if it is a good approach, I'd appreciate any thoughts or another suggestion

@Darksonn
Copy link
Contributor

Sigh ... parking_lot strikes again.

What types have a difference with parking_lot and miri?

@Darksonn
Copy link
Contributor

We had issues with this previously for Send with #4359.

@Darksonn
Copy link
Contributor

The solution here is probably to change some types in src/loom so that it implements the same set of traits no matter the parking_lot situation.

I would not look at miri until we address parking_lot. I suspect it may be the same issue.

sobczal2 added 4 commits July 29, 2025 21:24
Adjust `tokio/tests/async_send_sync.rs` tests to correctly assert
`UnwindSafe` and `RefUnwindSafe`. Remove `Cell` from example type since
it was causing differences just because it contained `UnsafeCell` and we
are aiming to test traits.
@sobczal2
Copy link
Contributor Author

sobczal2 commented Jul 29, 2025

As suggested, I changed the types in src/loom/std/parking_lot.rs, so now there is no difference between the std and parking_lot wrappers in terms of those traits. The biggest problem now is tokio::sync::Semaphore, which, for a reason I haven't yet understood, is UnwindSafe and RefUnwindSend when it doesn't use parking_lot, but stops implementing both when using parking_lot (this is temporarily fixed with impl UnwindSafe on linked_list, but should probably be changed). For sure tokio::sync::Semaphore and maybe other types also have a different problem – when tokio_unstable and tracing are enabled, they have a tracing::Span field, which is neither UnwindSafe nor RefUnwindSend. I'm not sure if this test can be done without further changes to tokio itself or a lot of conditional code.

@Darksonn
Copy link
Contributor

@hawkw Are you able to comment on the unwind safety of tracing::Span?

@@ -29,6 +30,7 @@ pub(crate) struct LinkedList<L, T> {

unsafe impl<L: Link> Send for LinkedList<L, L::Target> where L::Target: Send {}
unsafe impl<L: Link> Sync for LinkedList<L, L::Target> where L::Target: Sync {}
impl<L: Link> UnwindSafe for LinkedList<L, L::Target> {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the linked list is just raw pointers, having manual impls for both traits is probably the right call.

@sobczal2
Copy link
Contributor Author

sobczal2 commented Aug 6, 2025

How should we continue with this? Should I add second impl for linked list and guard tests for types containing tracing::Span with cfgs for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add asserts for UnwindSafe and RefUnwindSafe in async_send_sync.rs
3 participants