From f842d7b505eb57ac589e47525eb14e9acc888c57 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Mon, 23 Oct 2023 16:40:58 -0500 Subject: [PATCH 01/10] Allow cloning `poison::Guard`s. (makes implementing `Mapped*Guard` easier) --- library/std/src/sync/poison.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/sync/poison.rs b/library/std/src/sync/poison.rs index 741312d5537e9..ababaa2f06635 100644 --- a/library/std/src/sync/poison.rs +++ b/library/std/src/sync/poison.rs @@ -55,6 +55,7 @@ impl Flag { } } +#[derive(Clone)] pub struct Guard { panicking: bool, } From 9be1321676dd3a90b180ce8e9b6ac2da1bd2272d Mon Sep 17 00:00:00 2001 From: Zachary S Date: Mon, 23 Oct 2023 16:26:35 -0500 Subject: [PATCH 02/10] Implement `MappedMutexGuard`. --- library/std/src/sync/mutex.rs | 193 ++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index b4ae6b7e07ebc..18bea6d58fba1 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -3,7 +3,10 @@ mod tests; use crate::cell::UnsafeCell; use crate::fmt; +use crate::marker::PhantomData; +use crate::mem::ManuallyDrop; use crate::ops::{Deref, DerefMut}; +use crate::ptr::NonNull; use crate::sync::{poison, LockResult, TryLockError, TryLockResult}; use crate::sys::locks as sys; @@ -213,6 +216,43 @@ impl !Send for MutexGuard<'_, T> {} #[stable(feature = "mutexguard", since = "1.19.0")] unsafe impl Sync for MutexGuard<'_, T> {} +/// An RAII mutex guard returned by `MutexGuard::map`, which can point to a +/// subfield of the protected data. When this structure is dropped (falls out +/// of scope), the lock will be unlocked. +/// +/// The main difference between `MappedMutexGuard` and [`MutexGuard`] is that the +/// former cannot be used with [`CondVar`], since that +/// could introduce soundness issues if the locked object is modified by another +/// thread while the `Mutex` is unlocked. +/// +/// The data protected by the mutex can be accessed through this guard via its +/// [`Deref`] and [`DerefMut`] implementations. +/// +/// This structure is created by the [`map`] and [`try_map`] methods on +/// [`MutexGuard`]. +/// +/// [`map`]: MutexGuard::map +/// [`try_map`]: MutexGuard::try_map +/// [`CondVar`]: crate::sync::CondVar +#[must_use = "if unused the Mutex will immediately unlock"] +#[must_not_suspend = "holding a MappedMutexGuard across suspend \ + points can cause deadlocks, delays, \ + and cause Futures to not implement `Send`"] +#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[clippy::has_significant_drop] +pub struct MappedMutexGuard<'a, T: ?Sized + 'a> { + data: NonNull, + inner: &'a sys::Mutex, + poison_flag: &'a poison::Flag, + poison: poison::Guard, + _variance: PhantomData<&'a mut T>, +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl !Send for MappedMutexGuard<'_, T> {} +#[unstable(feature = "mapped_lock_guards", issue = "none")] +unsafe impl Sync for MappedMutexGuard<'_, T> {} + impl Mutex { /// Creates a new mutex in an unlocked state ready for use. /// @@ -552,3 +592,156 @@ pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::Mutex { pub fn guard_poison<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a poison::Flag { &guard.lock.poison } + +impl<'a, T: ?Sized> MutexGuard<'a, T> { + /// Makes a [`MappedMutexGuard`] for a component of the borrowed data, e.g. + /// an enum variant. + /// + /// The `Mutex` is already locked, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MutexGuard::map(...)`. A method would interfere with methods of the + /// same name on the contents of the `MutexGuard` used through `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&mut *orig)); + MappedMutexGuard { + data: value, + inner: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + } + } + + /// Makes a [`MappedMutexGuard`] for a component of the borrowed data. The + /// original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `Mutex` is already locked, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MutexGuard::try_map(...)`. A method would interfere with methods of the + /// same name on the contents of the `MutexGuard` used through `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&mut T) -> Option<&mut U>, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + match f(&mut *orig).map(NonNull::from) { + Some(value) => Ok(MappedMutexGuard { + data: value, + inner: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Deref for MappedMutexGuard<'_, T> { + type Target = T; + + fn deref(&self) -> &T { + unsafe { self.data.as_ref() } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl DerefMut for MappedMutexGuard<'_, T> { + fn deref_mut(&mut self) -> &mut T { + unsafe { self.data.as_mut() } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Drop for MappedMutexGuard<'_, T> { + #[inline] + fn drop(&mut self) { + unsafe { + self.poison_flag.done(&self.poison); + self.inner.unlock(); + } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Debug for MappedMutexGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Debug::fmt(&**self, f) + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Display for MappedMutexGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { + /// Makes a [`MappedMutexGuard`] for a component of the borrowed data, e.g. + /// an enum variant. + /// + /// The `Mutex` is already locked, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MutexGuard::map(...)`. A method would interfere with methods of the + /// same name on the contents of the `MutexGuard` used through `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&mut *orig)); + MappedMutexGuard { + data: value, + inner: orig.inner, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + } + } + + /// Makes a [`MappedMutexGuard`] for a component of the borrowed data. The + /// original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `Mutex` is already locked, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MutexGuard::try_map(...)`. A method would interfere with methods of the + /// same name on the contents of the `MutexGuard` used through `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&mut T) -> Option<&mut U>, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + match f(&mut *orig).map(NonNull::from) { + Some(value) => Ok(MappedMutexGuard { + data: value, + inner: orig.inner, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} From ea97c1f2dc451ddd50c159cf04967f9be742de8a Mon Sep 17 00:00:00 2001 From: Zachary S Date: Mon, 23 Oct 2023 16:26:59 -0500 Subject: [PATCH 03/10] Implmement `MappedRwLock(Read|Write)Guard`. --- library/std/src/sync/rwlock.rs | 358 ++++++++++++++++++++++++++++++++- 1 file changed, 355 insertions(+), 3 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 5d8967bfbe686..717f40db4c1c6 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -3,6 +3,8 @@ mod tests; use crate::cell::UnsafeCell; use crate::fmt; +use crate::marker::PhantomData; +use crate::mem::ManuallyDrop; use crate::ops::{Deref, DerefMut}; use crate::ptr::NonNull; use crate::sync::{poison, LockResult, TryLockError, TryLockResult}; @@ -105,7 +107,7 @@ unsafe impl Sync for RwLock {} #[cfg_attr(not(test), rustc_diagnostic_item = "RwLockReadGuard")] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a - // `Ref` argument doesn't hold immutability for its whole scope, only until it drops. + // `RwLockReadGuard` argument doesn't hold immutability for its whole scope, only until it drops. // `NonNull` is also covariant over `T`, just like we would have with `&T`. `NonNull` // is preferable over `const* T` to allow for niche optimization. data: NonNull, @@ -144,6 +146,63 @@ impl !Send for RwLockWriteGuard<'_, T> {} #[stable(feature = "rwlock_guard_sync", since = "1.23.0")] unsafe impl Sync for RwLockWriteGuard<'_, T> {} +/// RAII structure used to release the shared read access of a lock when +/// dropped, which can point to a subfield of the protected data. +/// +/// This structure is created by the [`map`] and [`try_map`] methods +/// on [`RwLockReadGuard`]. +/// +/// [`map`]: RwLockReadGuard::map +/// [`try_map`]: RwLockReadGuard::try_map +#[must_use = "if unused the RwLock will immediately unlock"] +#[must_not_suspend = "holding a MappedRwLockReadGuard across suspend \ + points can cause deadlocks, delays, \ + and cause Futures to not implement `Send`"] +#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[clippy::has_significant_drop] +pub struct MappedRwLockReadGuard<'a, T: ?Sized + 'a> { + // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a + // `MappedRwLockReadGuard` argument doesn't hold immutability for its whole scope, only until it drops. + // `NonNull` is also covariant over `T`, just like we would have with `&T`. `NonNull` + // is preferable over `const* T` to allow for niche optimization. + data: NonNull, + inner_lock: &'a sys::RwLock, +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl !Send for MappedRwLockReadGuard<'_, T> {} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +unsafe impl Sync for MappedRwLockReadGuard<'_, T> {} + +/// RAII structure used to release the exclusive write access of a lock when +/// dropped, which can point to a subfield of the protected data. +/// +/// This structure is created by the [`map`] and [`try_map`] methods +/// on [`RwLockWriteGuard`]. +/// +/// [`map`]: RwLockWriteGuard::map +/// [`try_map`]: RwLockWriteGuard::try_map +#[must_use = "if unused the RwLock will immediately unlock"] +#[must_not_suspend = "holding a MappedRwLockWriteGuard across suspend \ + points can cause deadlocks, delays, \ + and cause Future's to not implement `Send`"] +#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[clippy::has_significant_drop] +pub struct MappedRwLockWriteGuard<'a, T: ?Sized + 'a> { + data: NonNull, + inner_lock: &'a sys::RwLock, + poison_flag: &'a poison::Flag, + poison: poison::Guard, + _variance: PhantomData<&'a mut T>, +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl !Send for MappedRwLockWriteGuard<'_, T> {} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +unsafe impl Sync for MappedRwLockWriteGuard<'_, T> {} + impl RwLock { /// Creates a new instance of an `RwLock` which is unlocked. /// @@ -527,7 +586,10 @@ impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> { // SAFETY: if and only if `lock.inner.write()` (or `lock.inner.try_write()`) has been // successfully called from the same thread before instantiating this object. unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { - poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard }) + poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { + lock, + poison: guard, + }) } } @@ -559,12 +621,40 @@ impl fmt::Display for RwLockWriteGuard<'_, T> { } } +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Debug for MappedRwLockReadGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Display for MappedRwLockReadGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Debug for MappedRwLockWriteGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl fmt::Display for MappedRwLockWriteGuard<'_, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (**self).fmt(f) + } +} + #[stable(feature = "rust1", since = "1.0.0")] impl Deref for RwLockReadGuard<'_, T> { type Target = T; fn deref(&self) -> &T { - // SAFETY: the conditions of `RwLockGuard::new` were satisfied when created. + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when created. unsafe { self.data.as_ref() } } } @@ -587,6 +677,37 @@ impl DerefMut for RwLockWriteGuard<'_, T> { } } +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Deref for MappedRwLockReadGuard<'_, T> { + type Target = T; + + fn deref(&self) -> &T { + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + unsafe { self.data.as_ref() } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Deref for MappedRwLockWriteGuard<'_, T> { + type Target = T; + + fn deref(&self) -> &T { + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + unsafe { self.data.as_ref() } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl DerefMut for MappedRwLockWriteGuard<'_, T> { + fn deref_mut(&mut self) -> &mut T { + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + unsafe { self.data.as_mut() } + } +} + #[stable(feature = "rust1", since = "1.0.0")] impl Drop for RwLockReadGuard<'_, T> { fn drop(&mut self) { @@ -607,3 +728,234 @@ impl Drop for RwLockWriteGuard<'_, T> { } } } + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Drop for MappedRwLockReadGuard<'_, T> { + fn drop(&mut self) { + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + unsafe { + self.inner_lock.read_unlock(); + } + } +} + +#[unstable(feature = "mapped_lock_guards", issue = "none")] +impl Drop for MappedRwLockWriteGuard<'_, T> { + fn drop(&mut self) { + self.poison_flag.done(&self.poison); + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + unsafe { + self.inner_lock.write_unlock(); + } + } +} + +impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { + /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data, e.g. + /// an enum variant. + /// + /// The `RwLock` is already locked for reading, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `RwLockReadGuard::map(...)`. A method would interfere with methods of + /// the same name on the contents of the `RwLockReadGuard` used through + /// `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> + where + F: FnOnce(&T) -> &U, + U: ?Sized, + { + let orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&*orig)); + MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock } + } + + /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data. The + /// original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `RwLock` is already locked for reading, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `RwLockReadGuard::try_map(...)`. A method would interfere with methods + /// of the same name on the contents of the `RwLockReadGuard` used through + /// `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&T) -> Option<&U>, + U: ?Sized, + { + let orig = ManuallyDrop::new(orig); + match f(&*orig).map(NonNull::from) { + Some(value) => Ok(MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} + +impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { + /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data, + /// e.g. an enum variant. + /// + /// The `RwLock` is already locked for reading, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MappedRwLockReadGuard::map(...)`. A method would interfere with + /// methods of the same name on the contents of the `MappedRwLockReadGuard` + /// used through `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> + where + F: FnOnce(&T) -> &U, + U: ?Sized, + { + let orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&*orig)); + MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock } + } + + /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data. + /// The original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `RwLock` is already locked for reading, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MappedRwLockReadGuard::try_map(...)`. A method would interfere with + /// methods of the same name on the contents of the `MappedRwLockReadGuard` + /// used through `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&T) -> Option<&U>, + U: ?Sized, + { + let orig = ManuallyDrop::new(orig); + match f(&*orig).map(NonNull::from) { + Some(value) => Ok(MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} + +impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { + /// Makes a [`MappedRwLockWriteGuard`] for a component of the borrowed data, e.g. + /// an enum variant. + /// + /// The `RwLock` is already locked for writing, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `RwLockWriteGuard::map(...)`. A method would interfere with methods of + /// the same name on the contents of the `RwLockWriteGuard` used through + /// `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&mut *orig)); + MappedRwLockWriteGuard { + data: value, + inner_lock: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + } + } + + /// Makes a [`MappedRwLockWriteGuard`] for a component of the borrowed data. The + /// original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `RwLock` is already locked for writing, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `RwLockWriteGuard::try_map(...)`. A method would interfere with methods + /// of the same name on the contents of the `RwLockWriteGuard` used through + /// `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&mut T) -> Option<&mut U>, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + match f(&mut *orig).map(NonNull::from) { + Some(value) => Ok(MappedRwLockWriteGuard { + data: value, + inner_lock: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} + +impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> { + /// Makes a [`MappedRwLockWriteGuard`] for a component of the borrowed data, + /// e.g. an enum variant. + /// + /// The `RwLock` is already locked for writing, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MappedRwLockWriteGuard::map(...)`. A method would interfere with + /// methods of the same name on the contents of the `MappedRwLockWriteGuard` + /// used through `Deref`. + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> + where + F: FnOnce(&mut T) -> &mut U, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + let value = NonNull::from(f(&mut *orig)); + MappedRwLockWriteGuard { + data: value, + inner_lock: orig.inner_lock, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + } + } + + /// Makes a [`MappedRwLockWriteGuard`] for a component of the borrowed data. + /// The original guard is returned as an `Err(...)` if the closure returns + /// `None`. + /// + /// The `RwLock` is already locked for writing, so this cannot fail. + /// + /// This is an associated function that needs to be used as + /// `MappedRwLockWriteGuard::try_map(...)`. A method would interfere with + /// methods of the same name on the contents of the `MappedRwLockWriteGuard` + /// used through `Deref`. + #[doc(alias = "filter_map")] + #[unstable(feature = "mapped_lock_guards", issue = "none")] + pub fn try_map(orig: Self, f: F) -> Result, Self> + where + F: FnOnce(&mut T) -> Option<&mut U>, + U: ?Sized, + { + let mut orig = ManuallyDrop::new(orig); + match f(&mut *orig).map(NonNull::from) { + Some(value) => Ok(MappedRwLockWriteGuard { + data: value, + inner_lock: orig.inner_lock, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + }), + None => Err(ManuallyDrop::into_inner(orig)), + } + } +} From 04f86304422cfc74a46ee7d53d04eb1e56572599 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Mon, 23 Oct 2023 16:36:13 -0500 Subject: [PATCH 04/10] Add comment about `Mapped(Mutex|RwLockWrite)Guard` variance. --- library/std/src/sync/mutex.rs | 28 +++++++++++-------- library/std/src/sync/rwlock.rs | 50 ++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 35 deletions(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 18bea6d58fba1..a435fba4f1ea2 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -238,9 +238,13 @@ unsafe impl Sync for MutexGuard<'_, T> {} #[must_not_suspend = "holding a MappedMutexGuard across suspend \ points can cause deadlocks, delays, \ and cause Futures to not implement `Send`"] -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] #[clippy::has_significant_drop] pub struct MappedMutexGuard<'a, T: ?Sized + 'a> { + // NB: we use a pointer instead of `&'a mut T` to avoid `noalias` violations, because a + // `MappedMutexGuard` argument doesn't hold uniqueness for its whole scope, only until it drops. + // `NonNull` is covariant over `T`, so we add a `PhantomData<&'a mut T>` field + // below for the correct variance over `T` (invariance). data: NonNull, inner: &'a sys::Mutex, poison_flag: &'a poison::Flag, @@ -248,9 +252,9 @@ pub struct MappedMutexGuard<'a, T: ?Sized + 'a> { _variance: PhantomData<&'a mut T>, } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl !Send for MappedMutexGuard<'_, T> {} -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] unsafe impl Sync for MappedMutexGuard<'_, T> {} impl Mutex { @@ -602,7 +606,7 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { /// This is an associated function that needs to be used as /// `MutexGuard::map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, @@ -629,7 +633,7 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { /// `MutexGuard::try_map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, @@ -649,7 +653,7 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Deref for MappedMutexGuard<'_, T> { type Target = T; @@ -658,14 +662,14 @@ impl Deref for MappedMutexGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl DerefMut for MappedMutexGuard<'_, T> { fn deref_mut(&mut self) -> &mut T { unsafe { self.data.as_mut() } } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Drop for MappedMutexGuard<'_, T> { #[inline] fn drop(&mut self) { @@ -676,14 +680,14 @@ impl Drop for MappedMutexGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Debug for MappedMutexGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Debug::fmt(&**self, f) } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Display for MappedMutexGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (**self).fmt(f) @@ -699,7 +703,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// This is an associated function that needs to be used as /// `MutexGuard::map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, @@ -726,7 +730,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// `MutexGuard::try_map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 717f40db4c1c6..def0c8a16c7ce 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -158,7 +158,7 @@ unsafe impl Sync for RwLockWriteGuard<'_, T> {} #[must_not_suspend = "holding a MappedRwLockReadGuard across suspend \ points can cause deadlocks, delays, \ and cause Futures to not implement `Send`"] -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] #[clippy::has_significant_drop] pub struct MappedRwLockReadGuard<'a, T: ?Sized + 'a> { // NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a @@ -169,10 +169,10 @@ pub struct MappedRwLockReadGuard<'a, T: ?Sized + 'a> { inner_lock: &'a sys::RwLock, } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl !Send for MappedRwLockReadGuard<'_, T> {} -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] unsafe impl Sync for MappedRwLockReadGuard<'_, T> {} /// RAII structure used to release the exclusive write access of a lock when @@ -187,9 +187,13 @@ unsafe impl Sync for MappedRwLockReadGuard<'_, T> {} #[must_not_suspend = "holding a MappedRwLockWriteGuard across suspend \ points can cause deadlocks, delays, \ and cause Future's to not implement `Send`"] -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] #[clippy::has_significant_drop] pub struct MappedRwLockWriteGuard<'a, T: ?Sized + 'a> { + // NB: we use a pointer instead of `&'a mut T` to avoid `noalias` violations, because a + // `MappedRwLockWriteGuard` argument doesn't hold uniqueness for its whole scope, only until it drops. + // `NonNull` is covariant over `T`, so we add a `PhantomData<&'a mut T>` field + // below for the correct variance over `T` (invariance). data: NonNull, inner_lock: &'a sys::RwLock, poison_flag: &'a poison::Flag, @@ -197,10 +201,10 @@ pub struct MappedRwLockWriteGuard<'a, T: ?Sized + 'a> { _variance: PhantomData<&'a mut T>, } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl !Send for MappedRwLockWriteGuard<'_, T> {} -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] unsafe impl Sync for MappedRwLockWriteGuard<'_, T> {} impl RwLock { @@ -621,28 +625,28 @@ impl fmt::Display for RwLockWriteGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Debug for MappedRwLockReadGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (**self).fmt(f) } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Display for MappedRwLockReadGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (**self).fmt(f) } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Debug for MappedRwLockWriteGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (**self).fmt(f) } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl fmt::Display for MappedRwLockWriteGuard<'_, T> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { (**self).fmt(f) @@ -677,7 +681,7 @@ impl DerefMut for RwLockWriteGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Deref for MappedRwLockReadGuard<'_, T> { type Target = T; @@ -688,7 +692,7 @@ impl Deref for MappedRwLockReadGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Deref for MappedRwLockWriteGuard<'_, T> { type Target = T; @@ -699,7 +703,7 @@ impl Deref for MappedRwLockWriteGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl DerefMut for MappedRwLockWriteGuard<'_, T> { fn deref_mut(&mut self) -> &mut T { // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard @@ -729,7 +733,7 @@ impl Drop for RwLockWriteGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Drop for MappedRwLockReadGuard<'_, T> { fn drop(&mut self) { // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard @@ -740,7 +744,7 @@ impl Drop for MappedRwLockReadGuard<'_, T> { } } -#[unstable(feature = "mapped_lock_guards", issue = "none")] +#[unstable(feature = "mapped_lock_guards", issue = "117108")] impl Drop for MappedRwLockWriteGuard<'_, T> { fn drop(&mut self) { self.poison_flag.done(&self.poison); @@ -762,7 +766,7 @@ impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { /// `RwLockReadGuard::map(...)`. A method would interfere with methods of /// the same name on the contents of the `RwLockReadGuard` used through /// `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> where F: FnOnce(&T) -> &U, @@ -784,7 +788,7 @@ impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { /// of the same name on the contents of the `RwLockReadGuard` used through /// `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&T) -> Option<&U>, @@ -808,7 +812,7 @@ impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { /// `MappedRwLockReadGuard::map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockReadGuard` /// used through `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> where F: FnOnce(&T) -> &U, @@ -830,7 +834,7 @@ impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { /// methods of the same name on the contents of the `MappedRwLockReadGuard` /// used through `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&T) -> Option<&U>, @@ -854,7 +858,7 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// `RwLockWriteGuard::map(...)`. A method would interfere with methods of /// the same name on the contents of the `RwLockWriteGuard` used through /// `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, @@ -882,7 +886,7 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// of the same name on the contents of the `RwLockWriteGuard` used through /// `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, @@ -912,7 +916,7 @@ impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> { /// `MappedRwLockWriteGuard::map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockWriteGuard` /// used through `Deref`. - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, @@ -940,7 +944,7 @@ impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> { /// methods of the same name on the contents of the `MappedRwLockWriteGuard` /// used through `Deref`. #[doc(alias = "filter_map")] - #[unstable(feature = "mapped_lock_guards", issue = "none")] + #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, From 20fa3a0d8f529b428eeacf7cce184345f200654d Mon Sep 17 00:00:00 2001 From: Zachary S Date: Mon, 23 Oct 2023 17:03:17 -0500 Subject: [PATCH 05/10] Fix Condvar typo, add public re-exports of Mapped*Guard. --- library/std/src/sync/mod.rs | 4 ++++ library/std/src/sync/mutex.rs | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/library/std/src/sync/mod.rs b/library/std/src/sync/mod.rs index f6a7c0a9f7549..ca62179e95b66 100644 --- a/library/std/src/sync/mod.rs +++ b/library/std/src/sync/mod.rs @@ -165,6 +165,8 @@ pub use core::sync::Exclusive; pub use self::barrier::{Barrier, BarrierWaitResult}; #[stable(feature = "rust1", since = "1.0.0")] pub use self::condvar::{Condvar, WaitTimeoutResult}; +#[unstable(feature = "mapped_lock_guards", issue = "117108")] +pub use self::mutex::MappedMutexGuard; #[stable(feature = "rust1", since = "1.0.0")] pub use self::mutex::{Mutex, MutexGuard}; #[stable(feature = "rust1", since = "1.0.0")] @@ -172,6 +174,8 @@ pub use self::mutex::{Mutex, MutexGuard}; pub use self::once::{Once, OnceState, ONCE_INIT}; #[stable(feature = "rust1", since = "1.0.0")] pub use self::poison::{LockResult, PoisonError, TryLockError, TryLockResult}; +#[unstable(feature = "mapped_lock_guards", issue = "117108")] +pub use self::rwlock::{MappedRwLockReadGuard, MappedRwLockWriteGuard}; #[stable(feature = "rust1", since = "1.0.0")] pub use self::rwlock::{RwLock, RwLockReadGuard, RwLockWriteGuard}; diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index a435fba4f1ea2..5a419f4bd8465 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -221,7 +221,7 @@ unsafe impl Sync for MutexGuard<'_, T> {} /// of scope), the lock will be unlocked. /// /// The main difference between `MappedMutexGuard` and [`MutexGuard`] is that the -/// former cannot be used with [`CondVar`], since that +/// former cannot be used with [`Condvar`], since that /// could introduce soundness issues if the locked object is modified by another /// thread while the `Mutex` is unlocked. /// @@ -233,7 +233,7 @@ unsafe impl Sync for MutexGuard<'_, T> {} /// /// [`map`]: MutexGuard::map /// [`try_map`]: MutexGuard::try_map -/// [`CondVar`]: crate::sync::CondVar +/// [`Condvar`]: crate::sync::Condvar #[must_use = "if unused the Mutex will immediately unlock"] #[must_not_suspend = "holding a MappedMutexGuard across suspend \ points can cause deadlocks, delays, \ From 5533606fe0def63a62a3f75be4eb2d87081a05c4 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Tue, 24 Oct 2023 13:32:53 -0500 Subject: [PATCH 06/10] Add MappedMutexGuard and MappedRwLock*Guard tests. --- library/std/src/sync/mutex/tests.rs | 30 +++++++- library/std/src/sync/rwlock/tests.rs | 105 ++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 2 deletions(-) diff --git a/library/std/src/sync/mutex/tests.rs b/library/std/src/sync/mutex/tests.rs index 1786a3c09ffb5..cf69813baa3c5 100644 --- a/library/std/src/sync/mutex/tests.rs +++ b/library/std/src/sync/mutex/tests.rs @@ -1,6 +1,6 @@ use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sync::mpsc::channel; -use crate::sync::{Arc, Condvar, Mutex}; +use crate::sync::{Arc, Condvar, MappedMutexGuard, Mutex, MutexGuard}; use crate::thread; struct Packet(Arc<(Mutex, Condvar)>); @@ -188,6 +188,21 @@ fn test_mutex_arc_poison() { assert!(arc.is_poisoned()); } +#[test] +fn test_mutex_arc_poison_mapped() { + let arc = Arc::new(Mutex::new(1)); + assert!(!arc.is_poisoned()); + let arc2 = arc.clone(); + let _ = thread::spawn(move || { + let lock = arc2.lock().unwrap(); + let lock = MutexGuard::map(lock, |val| val); + assert_eq!(*lock, 2); // deliberate assertion failure to poison the mutex + }) + .join(); + assert!(arc.lock().is_err()); + assert!(arc.is_poisoned()); +} + #[test] fn test_mutex_arc_nested() { // Tests nested mutexes and access @@ -236,3 +251,16 @@ fn test_mutex_unsized() { let comp: &[i32] = &[4, 2, 5]; assert_eq!(&*mutex.lock().unwrap(), comp); } + +#[test] +fn test_mapping_mapped_guard() { + let arr = [0; 4]; + let mut lock = Mutex::new(arr); + let guard = lock.lock().unwrap(); + let guard = MutexGuard::map(guard, |arr| &mut arr[..2]); + let mut guard = MappedMutexGuard::map(guard, |slice| &mut slice[1..]); + assert_eq!(guard.len(), 1); + guard[0] = 42; + drop(guard); + assert_eq!(*lock.get_mut().unwrap(), [0, 42, 0, 0]); +} diff --git a/library/std/src/sync/rwlock/tests.rs b/library/std/src/sync/rwlock/tests.rs index 1a9d3d3f12f3c..0a5eb7aac023f 100644 --- a/library/std/src/sync/rwlock/tests.rs +++ b/library/std/src/sync/rwlock/tests.rs @@ -1,6 +1,9 @@ use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sync::mpsc::channel; -use crate::sync::{Arc, RwLock, RwLockReadGuard, TryLockError}; +use crate::sync::{ + Arc, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard, RwLockWriteGuard, + TryLockError, +}; use crate::thread; use rand::Rng; @@ -55,6 +58,19 @@ fn test_rw_arc_poison_wr() { assert!(arc.read().is_err()); } +#[test] +fn test_rw_arc_poison_mapped_w_r() { + let arc = Arc::new(RwLock::new(1)); + let arc2 = arc.clone(); + let _: Result<(), _> = thread::spawn(move || { + let lock = arc2.write().unwrap(); + let _lock = RwLockWriteGuard::map(lock, |val| val); + panic!(); + }) + .join(); + assert!(arc.read().is_err()); +} + #[test] fn test_rw_arc_poison_ww() { let arc = Arc::new(RwLock::new(1)); @@ -69,6 +85,20 @@ fn test_rw_arc_poison_ww() { assert!(arc.is_poisoned()); } +#[test] +fn test_rw_arc_poison_mapped_w_w() { + let arc = Arc::new(RwLock::new(1)); + let arc2 = arc.clone(); + let _: Result<(), _> = thread::spawn(move || { + let lock = arc2.write().unwrap(); + let _lock = RwLockWriteGuard::map(lock, |val| val); + panic!(); + }) + .join(); + assert!(arc.write().is_err()); + assert!(arc.is_poisoned()); +} + #[test] fn test_rw_arc_no_poison_rr() { let arc = Arc::new(RwLock::new(1)); @@ -81,6 +111,21 @@ fn test_rw_arc_no_poison_rr() { let lock = arc.read().unwrap(); assert_eq!(*lock, 1); } + +#[test] +fn test_rw_arc_no_poison_mapped_r_r() { + let arc = Arc::new(RwLock::new(1)); + let arc2 = arc.clone(); + let _: Result<(), _> = thread::spawn(move || { + let lock = arc2.read().unwrap(); + let _lock = RwLockReadGuard::map(lock, |val| val); + panic!(); + }) + .join(); + let lock = arc.read().unwrap(); + assert_eq!(*lock, 1); +} + #[test] fn test_rw_arc_no_poison_rw() { let arc = Arc::new(RwLock::new(1)); @@ -94,6 +139,20 @@ fn test_rw_arc_no_poison_rw() { assert_eq!(*lock, 1); } +#[test] +fn test_rw_arc_no_poison_mapped_r_w() { + let arc = Arc::new(RwLock::new(1)); + let arc2 = arc.clone(); + let _: Result<(), _> = thread::spawn(move || { + let lock = arc2.read().unwrap(); + let _lock = RwLockReadGuard::map(lock, |val| val); + panic!(); + }) + .join(); + let lock = arc.write().unwrap(); + assert_eq!(*lock, 1); +} + #[test] fn test_rw_arc() { let arc = Arc::new(RwLock::new(0)); @@ -179,6 +238,16 @@ fn test_rwlock_try_write() { } drop(read_guard); + let mapped_read_guard = RwLockReadGuard::map(lock.read().unwrap(), |_| &()); + + let write_result = lock.try_write(); + match write_result { + Err(TryLockError::WouldBlock) => (), + Ok(_) => assert!(false, "try_write should not succeed while mapped_read_guard is in scope"), + Err(_) => assert!(false, "unexpected error"), + } + + drop(mapped_read_guard); } #[test] @@ -257,3 +326,37 @@ fn test_read_guard_covariance() { } drop(lock); } + +#[test] +fn test_mapped_read_guard_covariance() { + fn do_stuff<'a>(_: MappedRwLockReadGuard<'_, &'a i32>, _: &'a i32) {} + let j: i32 = 5; + let lock = RwLock::new((&j, &j)); + { + let i = 6; + let guard = lock.read().unwrap(); + let guard = RwLockReadGuard::map(guard, |(val, _val)| val); + do_stuff(guard, &i); + } + drop(lock); +} + +#[test] +fn test_mapping_mapped_guard() { + let arr = [0; 4]; + let mut lock = RwLock::new(arr); + let guard = lock.write().unwrap(); + let guard = RwLockWriteGuard::map(guard, |arr| &mut arr[..2]); + let mut guard = MappedRwLockWriteGuard::map(guard, |slice| &mut slice[1..]); + assert_eq!(guard.len(), 1); + guard[0] = 42; + drop(guard); + assert_eq!(*lock.get_mut().unwrap(), [0, 42, 0, 0]); + + let guard = lock.read().unwrap(); + let guard = RwLockReadGuard::map(guard, |arr| &arr[..2]); + let guard = MappedRwLockReadGuard::map(guard, |slice| &slice[1..]); + assert_eq!(*guard, [42]); + drop(guard); + assert_eq!(*lock.get_mut().unwrap(), [0, 42, 0, 0]); +} From 6aebcbee0a4d232f675eb380269bb405b0a1cc32 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Tue, 24 Oct 2023 14:09:04 -0500 Subject: [PATCH 07/10] fix MappedMutexGuard::(try_)map doc typo. --- library/std/src/sync/mutex.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 5a419f4bd8465..1b3b4a3027cd7 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -701,7 +701,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// The `Mutex` is already locked, so this cannot fail. /// /// This is an associated function that needs to be used as - /// `MutexGuard::map(...)`. A method would interfere with methods of the + /// `MappedMutexGuard::map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> @@ -727,7 +727,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// The `Mutex` is already locked, so this cannot fail. /// /// This is an associated function that needs to be used as - /// `MutexGuard::try_map(...)`. A method would interfere with methods of the + /// `MappedMutexGuard::try_map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] From 3ef4b083ac03fd25339be009e3ae525adab30d78 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Thu, 26 Oct 2023 13:33:54 -0500 Subject: [PATCH 08/10] Specify behavior if the closure passed to *Guard::*map panics. --- library/std/src/sync/mutex.rs | 78 +++++++++----- library/std/src/sync/mutex/tests.rs | 63 ++++++++++- library/std/src/sync/rwlock.rs | 156 ++++++++++++++++++++------- library/std/src/sync/rwlock/tests.rs | 136 +++++++++++++++++++++++ 4 files changed, 364 insertions(+), 69 deletions(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 1b3b4a3027cd7..1b05247bb7c7c 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -612,10 +612,14 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&mut *orig)); + // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { &mut *orig.lock.data.get() })); + let orig = ManuallyDrop::new(orig); MappedMutexGuard { - data: value, + data, inner: &orig.lock.inner, poison_flag: &orig.lock.poison, poison: orig.poison.clone(), @@ -639,16 +643,23 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { F: FnOnce(&mut T) -> Option<&mut U>, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - match f(&mut *orig).map(NonNull::from) { - Some(value) => Ok(MappedMutexGuard { - data: value, - inner: &orig.lock.inner, - poison_flag: &orig.lock.poison, - poison: orig.poison.clone(), - _variance: PhantomData, - }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `MutexGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { &mut *orig.lock.data.get() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedMutexGuard { + data, + inner: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + }) + } + None => Err(orig), } } } @@ -704,15 +715,19 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// `MappedMutexGuard::map(...)`. A method would interfere with methods of the /// same name on the contents of the `MutexGuard` used through `Deref`. #[unstable(feature = "mapped_lock_guards", issue = "117108")] - pub fn map(orig: Self, f: F) -> MappedMutexGuard<'a, U> + pub fn map(mut orig: Self, f: F) -> MappedMutexGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&mut *orig)); + // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { orig.data.as_mut() })); + let orig = ManuallyDrop::new(orig); MappedMutexGuard { - data: value, + data, inner: orig.inner, poison_flag: orig.poison_flag, poison: orig.poison.clone(), @@ -731,21 +746,28 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { /// same name on the contents of the `MutexGuard` used through `Deref`. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] - pub fn try_map(orig: Self, f: F) -> Result, Self> + pub fn try_map(mut orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - match f(&mut *orig).map(NonNull::from) { - Some(value) => Ok(MappedMutexGuard { - data: value, - inner: orig.inner, - poison_flag: orig.poison_flag, - poison: orig.poison.clone(), - _variance: PhantomData, - }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { orig.data.as_mut() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedMutexGuard { + data, + inner: orig.inner, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + }) + } + None => Err(orig), } } } diff --git a/library/std/src/sync/mutex/tests.rs b/library/std/src/sync/mutex/tests.rs index cf69813baa3c5..19ec096c59334 100644 --- a/library/std/src/sync/mutex/tests.rs +++ b/library/std/src/sync/mutex/tests.rs @@ -1,6 +1,6 @@ use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sync::mpsc::channel; -use crate::sync::{Arc, Condvar, MappedMutexGuard, Mutex, MutexGuard}; +use crate::sync::{Arc, Condvar, MappedMutexGuard, Mutex, MutexGuard, TryLockError}; use crate::thread; struct Packet(Arc<(Mutex, Condvar)>); @@ -264,3 +264,64 @@ fn test_mapping_mapped_guard() { drop(guard); assert_eq!(*lock.get_mut().unwrap(), [0, 42, 0, 0]); } + +#[test] +fn panic_while_mapping_unlocked_poison() { + let lock = Mutex::new(()); + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.lock().unwrap(); + let _guard = MutexGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_lock() { + Ok(_) => panic!("panicking in a MutexGuard::map closure should poison the Mutex"), + Err(TryLockError::WouldBlock) => { + panic!("panicking in a MutexGuard::map closure should unlock the mutex") + } + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.lock().unwrap(); + let _guard = MutexGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_lock() { + Ok(_) => panic!("panicking in a MutexGuard::try_map closure should poison the Mutex"), + Err(TryLockError::WouldBlock) => { + panic!("panicking in a MutexGuard::try_map closure should unlock the mutex") + } + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.lock().unwrap(); + let guard = MutexGuard::map::<(), _>(guard, |val| val); + let _guard = MappedMutexGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_lock() { + Ok(_) => panic!("panicking in a MappedMutexGuard::map closure should poison the Mutex"), + Err(TryLockError::WouldBlock) => { + panic!("panicking in a MappedMutexGuard::map closure should unlock the mutex") + } + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.lock().unwrap(); + let guard = MutexGuard::map::<(), _>(guard, |val| val); + let _guard = MappedMutexGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_lock() { + Ok(_) => panic!("panicking in a MappedMutexGuard::try_map closure should poison the Mutex"), + Err(TryLockError::WouldBlock) => { + panic!("panicking in a MappedMutexGuard::try_map closure should unlock the mutex") + } + Err(TryLockError::Poisoned(_)) => {} + } + + drop(lock); +} diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index def0c8a16c7ce..5c4e4a784dbf5 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -766,15 +766,23 @@ impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { /// `RwLockReadGuard::map(...)`. A method would interfere with methods of /// the same name on the contents of the `RwLockReadGuard` used through /// `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will not be poisoned. #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> where F: FnOnce(&T) -> &U, U: ?Sized, { + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { orig.data.as_ref() })); let orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&*orig)); - MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock } + MappedRwLockReadGuard { data, inner_lock: &orig.inner_lock } } /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data. The @@ -787,6 +795,10 @@ impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { /// `RwLockReadGuard::try_map(...)`. A method would interfere with methods /// of the same name on the contents of the `RwLockReadGuard` used through /// `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will not be poisoned. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> @@ -794,10 +806,17 @@ impl<'a, T: ?Sized> RwLockReadGuard<'a, T> { F: FnOnce(&T) -> Option<&U>, U: ?Sized, { - let orig = ManuallyDrop::new(orig); - match f(&*orig).map(NonNull::from) { - Some(value) => Ok(MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { orig.data.as_ref() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedRwLockReadGuard { data, inner_lock: &orig.inner_lock }) + } + None => Err(orig), } } } @@ -812,15 +831,23 @@ impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { /// `MappedRwLockReadGuard::map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockReadGuard` /// used through `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will not be poisoned. #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockReadGuard<'a, U> where F: FnOnce(&T) -> &U, U: ?Sized, { + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { orig.data.as_ref() })); let orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&*orig)); - MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock } + MappedRwLockReadGuard { data, inner_lock: &orig.inner_lock } } /// Makes a [`MappedRwLockReadGuard`] for a component of the borrowed data. @@ -833,6 +860,10 @@ impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { /// `MappedRwLockReadGuard::try_map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockReadGuard` /// used through `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will not be poisoned. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> @@ -840,10 +871,17 @@ impl<'a, T: ?Sized> MappedRwLockReadGuard<'a, T> { F: FnOnce(&T) -> Option<&U>, U: ?Sized, { - let orig = ManuallyDrop::new(orig); - match f(&*orig).map(NonNull::from) { - Some(value) => Ok(MappedRwLockReadGuard { data: value, inner_lock: &orig.inner_lock }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { orig.data.as_ref() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedRwLockReadGuard { data, inner_lock: &orig.inner_lock }) + } + None => Err(orig), } } } @@ -858,16 +896,24 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// `RwLockWriteGuard::map(...)`. A method would interfere with methods of /// the same name on the contents of the `RwLockWriteGuard` used through /// `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will be poisoned. #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&mut *orig)); + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { &mut *orig.lock.data.get() })); + let orig = ManuallyDrop::new(orig); MappedRwLockWriteGuard { - data: value, + data, inner_lock: &orig.lock.inner, poison_flag: &orig.lock.poison, poison: orig.poison.clone(), @@ -885,6 +931,10 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// `RwLockWriteGuard::try_map(...)`. A method would interfere with methods /// of the same name on the contents of the `RwLockWriteGuard` used through /// `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will be poisoned. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] pub fn try_map(orig: Self, f: F) -> Result, Self> @@ -892,16 +942,23 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { F: FnOnce(&mut T) -> Option<&mut U>, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - match f(&mut *orig).map(NonNull::from) { - Some(value) => Ok(MappedRwLockWriteGuard { - data: value, - inner_lock: &orig.lock.inner, - poison_flag: &orig.lock.poison, - poison: orig.poison.clone(), - _variance: PhantomData, - }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { &mut *orig.lock.data.get() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedRwLockWriteGuard { + data, + inner_lock: &orig.lock.inner, + poison_flag: &orig.lock.poison, + poison: orig.poison.clone(), + _variance: PhantomData, + }) + } + None => Err(orig), } } } @@ -916,16 +973,24 @@ impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> { /// `MappedRwLockWriteGuard::map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockWriteGuard` /// used through `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will be poisoned. #[unstable(feature = "mapped_lock_guards", issue = "117108")] - pub fn map(orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> + pub fn map(mut orig: Self, f: F) -> MappedRwLockWriteGuard<'a, U> where F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - let value = NonNull::from(f(&mut *orig)); + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + let data = NonNull::from(f(unsafe { orig.data.as_mut() })); + let orig = ManuallyDrop::new(orig); MappedRwLockWriteGuard { - data: value, + data, inner_lock: orig.inner_lock, poison_flag: orig.poison_flag, poison: orig.poison.clone(), @@ -943,23 +1008,34 @@ impl<'a, T: ?Sized> MappedRwLockWriteGuard<'a, T> { /// `MappedRwLockWriteGuard::try_map(...)`. A method would interfere with /// methods of the same name on the contents of the `MappedRwLockWriteGuard` /// used through `Deref`. + /// + /// # Panics + /// + /// If the closure panics, the guard will be dropped (unlocked) and the RwLock will be poisoned. #[doc(alias = "filter_map")] #[unstable(feature = "mapped_lock_guards", issue = "117108")] - pub fn try_map(orig: Self, f: F) -> Result, Self> + pub fn try_map(mut orig: Self, f: F) -> Result, Self> where F: FnOnce(&mut T) -> Option<&mut U>, U: ?Sized, { - let mut orig = ManuallyDrop::new(orig); - match f(&mut *orig).map(NonNull::from) { - Some(value) => Ok(MappedRwLockWriteGuard { - data: value, - inner_lock: orig.inner_lock, - poison_flag: orig.poison_flag, - poison: orig.poison.clone(), - _variance: PhantomData, - }), - None => Err(ManuallyDrop::into_inner(orig)), + // SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when the original guard + // was created, and have been upheld throughout `map` and/or `try_map`. + // The signature of the closure guarantees that it will not "leak" the lifetime of the reference + // passed to it. If the closure panics, the guard will be dropped. + match f(unsafe { orig.data.as_mut() }) { + Some(data) => { + let data = NonNull::from(data); + let orig = ManuallyDrop::new(orig); + Ok(MappedRwLockWriteGuard { + data, + inner_lock: orig.inner_lock, + poison_flag: orig.poison_flag, + poison: orig.poison.clone(), + _variance: PhantomData, + }) + } + None => Err(orig), } } } diff --git a/library/std/src/sync/rwlock/tests.rs b/library/std/src/sync/rwlock/tests.rs index 0a5eb7aac023f..9cc5e7a3a60f1 100644 --- a/library/std/src/sync/rwlock/tests.rs +++ b/library/std/src/sync/rwlock/tests.rs @@ -360,3 +360,139 @@ fn test_mapping_mapped_guard() { drop(guard); assert_eq!(*lock.get_mut().unwrap(), [0, 42, 0, 0]); } + +#[test] +fn panic_while_mapping_read_unlocked_no_poison() { + let lock = RwLock::new(()); + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.read().unwrap(); + let _guard = RwLockReadGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => {} + Err(TryLockError::WouldBlock) => { + panic!("panicking in a RwLockReadGuard::map closure should release the read lock") + } + Err(TryLockError::Poisoned(_)) => { + panic!("panicking in a RwLockReadGuard::map closure should not poison the RwLock") + } + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.read().unwrap(); + let _guard = RwLockReadGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => {} + Err(TryLockError::WouldBlock) => { + panic!("panicking in a RwLockReadGuard::try_map closure should release the read lock") + } + Err(TryLockError::Poisoned(_)) => { + panic!("panicking in a RwLockReadGuard::try_map closure should not poison the RwLock") + } + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.read().unwrap(); + let guard = RwLockReadGuard::map::<(), _>(guard, |val| val); + let _guard = MappedRwLockReadGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => {} + Err(TryLockError::WouldBlock) => { + panic!("panicking in a MappedRwLockReadGuard::map closure should release the read lock") + } + Err(TryLockError::Poisoned(_)) => { + panic!("panicking in a MappedRwLockReadGuard::map closure should not poison the RwLock") + } + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.read().unwrap(); + let guard = RwLockReadGuard::map::<(), _>(guard, |val| val); + let _guard = MappedRwLockReadGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => {} + Err(TryLockError::WouldBlock) => panic!( + "panicking in a MappedRwLockReadGuard::try_map closure should release the read lock" + ), + Err(TryLockError::Poisoned(_)) => panic!( + "panicking in a MappedRwLockReadGuard::try_map closure should not poison the RwLock" + ), + } + + drop(lock); +} + +#[test] +fn panic_while_mapping_write_unlocked_poison() { + let lock = RwLock::new(()); + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.write().unwrap(); + let _guard = RwLockWriteGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => panic!("panicking in a RwLockWriteGuard::map closure should poison the RwLock"), + Err(TryLockError::WouldBlock) => { + panic!("panicking in a RwLockWriteGuard::map closure should release the write lock") + } + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.write().unwrap(); + let _guard = RwLockWriteGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => { + panic!("panicking in a RwLockWriteGuard::try_map closure should poison the RwLock") + } + Err(TryLockError::WouldBlock) => { + panic!("panicking in a RwLockWriteGuard::try_map closure should release the write lock") + } + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.write().unwrap(); + let guard = RwLockWriteGuard::map::<(), _>(guard, |val| val); + let _guard = MappedRwLockWriteGuard::map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => { + panic!("panicking in a MappedRwLockWriteGuard::map closure should poison the RwLock") + } + Err(TryLockError::WouldBlock) => panic!( + "panicking in a MappedRwLockWriteGuard::map closure should release the write lock" + ), + Err(TryLockError::Poisoned(_)) => {} + } + + let _ = crate::panic::catch_unwind(|| { + let guard = lock.write().unwrap(); + let guard = RwLockWriteGuard::map::<(), _>(guard, |val| val); + let _guard = MappedRwLockWriteGuard::try_map::<(), _>(guard, |_| panic!()); + }); + + match lock.try_write() { + Ok(_) => panic!( + "panicking in a MappedRwLockWriteGuard::try_map closure should poison the RwLock" + ), + Err(TryLockError::WouldBlock) => panic!( + "panicking in a MappedRwLockWriteGuard::try_map closure should release the write lock" + ), + Err(TryLockError::Poisoned(_)) => {} + } + + drop(lock); +} From 06ec7a7611bfcfd228507988a325f229d425598f Mon Sep 17 00:00:00 2001 From: Zachary S Date: Tue, 5 Dec 2023 18:10:39 -0600 Subject: [PATCH 09/10] fmt --- library/std/src/sync/rwlock.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 5c4e4a784dbf5..b985988d760c9 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -590,10 +590,7 @@ impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> { // SAFETY: if and only if `lock.inner.write()` (or `lock.inner.try_write()`) has been // successfully called from the same thread before instantiating this object. unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { - poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { - lock, - poison: guard, - }) + poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard }) } } From 8aaa04b5c5c1d47ee4c4622908f303b8b9632979 Mon Sep 17 00:00:00 2001 From: zachs18 <8355914+zachs18@users.noreply.github.com> Date: Fri, 23 Feb 2024 20:18:04 -0600 Subject: [PATCH 10/10] Apply suggestions from code review Co-authored-by: Amanieu d'Antras --- library/std/src/sync/mutex.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 1b05247bb7c7c..7b4b53e83dbab 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -612,7 +612,7 @@ impl<'a, T: ?Sized> MutexGuard<'a, T> { F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // SAFETY: the conditions of `MutexGuard::new` were satisfied when the original guard // was created, and have been upheld throughout `map` and/or `try_map`. // The signature of the closure guarantees that it will not "leak" the lifetime of the reference // passed to it. If the closure panics, the guard will be dropped. @@ -720,7 +720,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { F: FnOnce(&mut T) -> &mut U, U: ?Sized, { - // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // SAFETY: the conditions of `MutexGuard::new` were satisfied when the original guard // was created, and have been upheld throughout `map` and/or `try_map`. // The signature of the closure guarantees that it will not "leak" the lifetime of the reference // passed to it. If the closure panics, the guard will be dropped. @@ -751,7 +751,7 @@ impl<'a, T: ?Sized> MappedMutexGuard<'a, T> { F: FnOnce(&mut T) -> Option<&mut U>, U: ?Sized, { - // SAFETY: the conditions of `MutedGuard::new` were satisfied when the original guard + // SAFETY: the conditions of `MutexGuard::new` were satisfied when the original guard // was created, and have been upheld throughout `map` and/or `try_map`. // The signature of the closure guarantees that it will not "leak" the lifetime of the reference // passed to it. If the closure panics, the guard will be dropped.