-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Don't inherit Send from parking_lot::*Guard #4359
Conversation
739c02b
to
993dc5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like the best solution to me. I left some minor nitpicks but didn't see any blockers.
tokio/src/loom/std/parking_lot.rs
Outdated
pub(crate) struct Mutex<T: ?Sized>(PhantomData<std::sync::Mutex<T>>, parking_lot::Mutex<T>); | ||
|
||
#[derive(Debug)] | ||
pub(crate) struct RwLock<T>(parking_lot::RwLock<T>); | ||
pub(crate) struct RwLock<T>(PhantomData<std::sync::RwLock<T>>, parking_lot::RwLock<T>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have a slight preference for either making these named fields, or breaking them across multiple lines like we do with the guard types...this feels like a lot for one line. not a blocker though.
Currently our
tokio::sync::watch::Ref
type can becomeSend
if Tokio is compiled with theparking_lot
feature andparking_lot
is compiled with itssend_guard
feature. This PR will prevent thesend_guard
feature from affecting whether Tokio types areSend
or not.Strictly speaking, this PR is a breaking change, however I believe we should make the change regardless. As it stands now, the
parking_lot
crate is in our public API, and if don't want to make this change, then we should also commit to never upgradingparking_lot
beyond0.11.x
, as such an upgrade would be also be a breaking change for the same reasons.I don't think many people will be affected by this. Anyone who is affected is probably holding a
tokio::sync::watch::Ref
alive across an.await
, which is always incorrect.The first commit in this PR adds a test to CI that should catch this in the future. The commit is first so we can see the test fail without the change. The test relies on
tokio/tokio/tests/async_send_sync.rs
.