From 91fe74acbfab0f9140e184a9607d1c11fe464440 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Tue, 7 Mar 2023 02:28:50 +0000 Subject: [PATCH 1/5] Add `RwLockWriteGuard::{downgrade_map, try_downgrade_map}`. `downgrade_map` can already be made using a combination of `RwLockWriteGuard::downgrade` and `RwLockReadGuard::map`, but this version runs `f` *before* downgrading, in case it's useful for anyone and just for completion's sake. The real use case is with `try_downgrade_map`. In this case, we run `f` *before* we downgrade, so we can return the write lock in case it returns `None`. This isn't implementable in safe code, at least not without one of the following tradeoffs: * Call `f` twice: Not always doable, when it's expensive or not idempotent. * Downgrade, call `f`, then re-lock if it returned `None`: If you're doing double-checked locking, this goes against the whole point of it, because you unlock and so state might have changed in the meantime. --- tokio/src/sync/rwlock/write_guard.rs | 115 +++++++++++++++++++- tokio/src/sync/rwlock/write_guard_mapped.rs | 3 + 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/tokio/src/sync/rwlock/write_guard.rs b/tokio/src/sync/rwlock/write_guard.rs index 4527e5581c9..45bd12b3ba2 100644 --- a/tokio/src/sync/rwlock/write_guard.rs +++ b/tokio/src/sync/rwlock/write_guard.rs @@ -102,7 +102,60 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { } } - /// Attempts to make a new [`RwLockMappedWriteGuard`] for a component of + /// Makes a new [`RwLockReadGuard`] for a component of the locked data. + /// + /// This operation cannot fail as the `RwLockWriteGuard` passed in already + /// locked the data. + /// + /// This is an associated function that needs to be used as + /// `RwLockWriteGuard::downgrade_map(..)`. A method would interfere with methods of + /// the same name on the contents of the locked data. + /// + /// This is equivalent to a combination of asynchronous [`RwLockWriteGuard::map`] and [`RwLockWriteGuard::downgrade`] + /// from the [`parking_lot` crate]. + /// + /// Inside of `f`, you retain exclusive access to the data, despite only being given a `&T`. Handing out a + /// `&mut T` would result in unsoundness, as you could use interior mutability. + /// + /// [`RwLockMappedWriteGuard`]: struct@crate::sync::RwLockMappedWriteGuard + /// [`RwLockWriteGuard::map`]: https://docs.rs/lock_api/latest/lock_api/struct.RwLockWriteGuard.html#method.map + /// [`RwLockWriteGuard::downgrade`]: https://docs.rs/lock_api/latest/lock_api/struct.RwLockWriteGuard.html#method.downgrade + /// [`parking_lot` crate]: https://crates.io/crates/parking_lot + /// + /// # Examples + /// + /// ``` + /// use tokio::sync::{RwLock, RwLockWriteGuard}; + /// + /// #[derive(Debug, Clone, Copy, PartialEq, Eq)] + /// struct Foo(u32); + /// + /// # #[tokio::main] + /// # async fn main() { + /// let lock = RwLock::new(Foo(1)); + /// + /// let mapped = RwLockWriteGuard::downgrade_map(lock.write().await, |f| &f.0); + /// assert_eq!(1, *mapped); + /// # } + /// ``` + #[inline] + pub fn downgrade_map(this: Self, f: F) -> RwLockReadGuard<'a, U> + where + F: FnOnce(&T) -> &U, + { + let data = f(&*this) as *const U; + let this = this.skip_drop(); + + RwLockReadGuard { + s: this.s, + data, + marker: PhantomData, + #[cfg(all(tokio_unstable, feature = "tracing"))] + resource_span: this.resource_span, + } + } + + /// Attempts to make a new [`RwLockMappedWriteGuard`] for a component of /// the locked data. The original guard is returned if the closure returns /// `None`. /// @@ -165,6 +218,66 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { }) } + /// Attempts to make a new [`RwLockReadGuard`] for a component of + /// the locked data. The original guard is returned if the closure returns + /// `None`. + /// + /// This operation cannot fail as the `RwLockWriteGuard` passed in already + /// locked the data. + /// + /// This is an associated function that needs to be + /// used as `RwLockWriteGuard::try_downgrade_map(...)`. A method would interfere with + /// methods of the same name on the contents of the locked data. + /// + /// This is equivalent to a combination of asynchronous [`RwLockWriteGuard::try_map`] and [`RwLockWriteGuard::downgrade`] + /// from the [`parking_lot` crate]. + /// + /// Inside of `f`, you retain exclusive access to the data, despite only being given a `&T`. Handing out a + /// `&mut T` would result in unsoundness, as you could use interior mutability. + /// + /// If this function returns `Err(...)`, the lock is never unlocked nor downgraded. + /// + /// [`RwLockMappedWriteGuard`]: struct@crate::sync::RwLockMappedWriteGuard + /// [`RwLockWriteGuard::map`]: https://docs.rs/lock_api/latest/lock_api/struct.RwLockWriteGuard.html#method.map + /// [`RwLockWriteGuard::downgrade`]: https://docs.rs/lock_api/latest/lock_api/struct.RwLockWriteGuard.html#method.downgrade + /// [`parking_lot` crate]: https://crates.io/crates/parking_lot + /// + /// # Examples + /// + /// ``` + /// use tokio::sync::{RwLock, RwLockWriteGuard}; + /// + /// #[derive(Debug, Clone, Copy, PartialEq, Eq)] + /// struct Foo(u32); + /// + /// # #[tokio::main] + /// # async fn main() { + /// let lock = RwLock::new(Foo(1)); + /// + /// let guard = RwLockWriteGuard::try_downgrade_map(lock.write().await, |f| Some(&f.0)).expect("should not fail"); + /// assert_eq!(1, *guard); + /// # } + /// ``` + #[inline] + pub fn try_downgrade_map(this: Self, f: F) -> Result, Self> + where + F: FnOnce(&T) -> Option<&U>, + { + let data = match f(&*this) { + Some(data) => data as *const U, + None => return Err(this), + }; + let this = this.skip_drop(); + + Ok(RwLockReadGuard { + s: this.s, + data, + marker: PhantomData, + #[cfg(all(tokio_unstable, feature = "tracing"))] + resource_span: this.resource_span, + }) + } + /// Converts this `RwLockWriteGuard` into an `RwLockMappedWriteGuard`. This /// method can be used to store a non-mapped guard in a struct field that /// expects a mapped guard. diff --git a/tokio/src/sync/rwlock/write_guard_mapped.rs b/tokio/src/sync/rwlock/write_guard_mapped.rs index 1248c4c7af3..7705189e70f 100644 --- a/tokio/src/sync/rwlock/write_guard_mapped.rs +++ b/tokio/src/sync/rwlock/write_guard_mapped.rs @@ -160,6 +160,9 @@ impl<'a, T: ?Sized> RwLockMappedWriteGuard<'a, T> { resource_span: this.resource_span, }) } + + // Note: No `downgrade`, `downgrade_map` nor `try_downgrade_map` because they would be unsound, as we're already + // potentially been mapped with internal mutability. } impl ops::Deref for RwLockMappedWriteGuard<'_, T> { From 180e2d33991d8b554f0b450dd73339ede6cf8183 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Tue, 7 Mar 2023 02:28:53 +0000 Subject: [PATCH 2/5] Fixed `RwLockWriteGuard::{downgrade_map, try_downgrade_map}` not properly downgrading. `RwLockWriteGuard::{downgrade_map, try_downgrade_map}` doctests now test whether a read lock can be obtained afterwards. --- tokio/src/sync/rwlock/write_guard.rs | 64 ++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 8 deletions(-) diff --git a/tokio/src/sync/rwlock/write_guard.rs b/tokio/src/sync/rwlock/write_guard.rs index 45bd12b3ba2..d405fc2b39f 100644 --- a/tokio/src/sync/rwlock/write_guard.rs +++ b/tokio/src/sync/rwlock/write_guard.rs @@ -135,7 +135,8 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// let lock = RwLock::new(Foo(1)); /// /// let mapped = RwLockWriteGuard::downgrade_map(lock.write().await, |f| &f.0); - /// assert_eq!(1, *mapped); + /// let foo = lock.read().await; + /// assert_eq!(foo.0, *mapped); /// # } /// ``` #[inline] @@ -145,14 +146,37 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { { let data = f(&*this) as *const U; let this = this.skip_drop(); - - RwLockReadGuard { + let guard = RwLockReadGuard { s: this.s, data, marker: PhantomData, #[cfg(all(tokio_unstable, feature = "tracing"))] resource_span: this.resource_span, - } + }; + + // Release all but one of the permits held by the write guard + let to_release = (this.permits_acquired - 1) as usize; + this.s.release(to_release); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + write_locked = false, + write_locked.op = "override", + ) + }); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + current_readers = 1, + current_readers.op = "add", + ) + }); + + guard } /// Attempts to make a new [`RwLockMappedWriteGuard`] for a component of @@ -255,7 +279,8 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { /// let lock = RwLock::new(Foo(1)); /// /// let guard = RwLockWriteGuard::try_downgrade_map(lock.write().await, |f| Some(&f.0)).expect("should not fail"); - /// assert_eq!(1, *guard); + /// let foo = lock.read().await; + /// assert_eq!(foo.0, *guard); /// # } /// ``` #[inline] @@ -268,14 +293,37 @@ impl<'a, T: ?Sized> RwLockWriteGuard<'a, T> { None => return Err(this), }; let this = this.skip_drop(); - - Ok(RwLockReadGuard { + let guard = RwLockReadGuard { s: this.s, data, marker: PhantomData, #[cfg(all(tokio_unstable, feature = "tracing"))] resource_span: this.resource_span, - }) + }; + + // Release all but one of the permits held by the write guard + let to_release = (this.permits_acquired - 1) as usize; + this.s.release(to_release); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + write_locked = false, + write_locked.op = "override", + ) + }); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + current_readers = 1, + current_readers.op = "add", + ) + }); + + Ok(guard) } /// Converts this `RwLockWriteGuard` into an `RwLockMappedWriteGuard`. This From 6f636d55d19a1fe6655d86ab2c9882a628d12f67 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Tue, 7 Mar 2023 02:28:53 +0000 Subject: [PATCH 3/5] Added `OwnedRwLockWriteGuard::{downgrade_map, try_downgrade_map}`. These are mirrors of the existing methods on `RwLockLockWriteGuard`. --- tokio/src/sync/rwlock/owned_write_guard.rs | 152 ++++++++++++++++++++- 1 file changed, 151 insertions(+), 1 deletion(-) diff --git a/tokio/src/sync/rwlock/owned_write_guard.rs b/tokio/src/sync/rwlock/owned_write_guard.rs index cbc77f6f4a2..a8d671e9781 100644 --- a/tokio/src/sync/rwlock/owned_write_guard.rs +++ b/tokio/src/sync/rwlock/owned_write_guard.rs @@ -100,7 +100,77 @@ impl OwnedRwLockWriteGuard { } } - /// Attempts to make a new [`OwnedRwLockMappedWriteGuard`] for a component + /// Makes a new [`OwnedRwLockReadGuard`] for a component of the locked data. + /// + /// This operation cannot fail as the `OwnedRwLockWriteGuard` passed in already + /// locked the data. + /// + /// This is an associated function that needs to be used as + /// `OwnedRwLockWriteGuard::downgrade_map(..)`. A method would interfere with methods of + /// the same name on the contents of the locked data. + /// + /// Inside of `f`, you retain exclusive access to the data, despite only being given a `&T`. Handing out a + /// `&mut T` would result in unsoundness, as you could use interior mutability. + /// + /// # Examples + /// + /// ``` + /// use std::sync::Arc; + /// use tokio::sync::{RwLock, OwnedRwLockWriteGuard}; + /// + /// #[derive(Debug, Clone, Copy, PartialEq, Eq)] + /// struct Foo(u32); + /// + /// # #[tokio::main] + /// # async fn main() { + /// let lock = Arc::new(RwLock::new(Foo(1))); + /// + /// let mapped = OwnedRwLockWriteGuard::downgrade_map(Arc::clone(&lock).write_owned().await, |f| &f.0); + /// let foo = lock.read_owned().await; + /// assert_eq!(foo.0, *mapped); + /// # } + /// ``` + #[inline] + pub fn downgrade_map(this: Self, f: F) -> OwnedRwLockReadGuard + where + F: FnOnce(&T) -> &U, + { + let data = f(&*this) as *const U; + let this = this.skip_drop(); + let guard = OwnedRwLockReadGuard { + lock: this.lock, + data, + _p: PhantomData, + #[cfg(all(tokio_unstable, feature = "tracing"))] + resource_span: this.resource_span, + }; + + // Release all but one of the permits held by the write guard + let to_release = (this.permits_acquired - 1) as usize; + guard.lock.s.release(to_release); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + write_locked = false, + write_locked.op = "override", + ) + }); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + current_readers = 1, + current_readers.op = "add", + ) + }); + + guard + } + + /// Attempts to make a new [`OwnedRwLockMappedWriteGuard`] for a component /// of the locked data. The original guard is returned if the closure /// returns `None`. /// @@ -159,6 +229,86 @@ impl OwnedRwLockWriteGuard { }) } + /// Attempts to make a new [`OwnedRwLockReadGuard`] for a component of + /// the locked data. The original guard is returned if the closure returns + /// `None`. + /// + /// This operation cannot fail as the `OwnedRwLockWriteGuard` passed in already + /// locked the data. + /// + /// This is an associated function that needs to be + /// used as `OwnedRwLockWriteGuard::try_downgrade_map(...)`. A method would interfere with + /// methods of the same name on the contents of the locked data. + /// + /// Inside of `f`, you retain exclusive access to the data, despite only being given a `&T`. Handing out a + /// `&mut T` would result in unsoundness, as you could use interior mutability. + /// + /// If this function returns `Err(...)`, the lock is never unlocked nor downgraded. + /// + /// # Examples + /// + /// ``` + /// use std::sync::Arc; + /// use tokio::sync::{RwLock, OwnedRwLockWriteGuard}; + /// + /// #[derive(Debug, Clone, Copy, PartialEq, Eq)] + /// struct Foo(u32); + /// + /// # #[tokio::main] + /// # async fn main() { + /// let lock = Arc::new(RwLock::new(Foo(1))); + /// + /// let guard = OwnedRwLockWriteGuard::try_downgrade_map(Arc::clone(&lock).write_owned().await, |f| Some(&f.0)).expect("should not fail"); + /// let foo = lock.read_owned().await; + /// assert_eq!(foo.0, *guard); + /// # } + /// ``` + #[inline] + pub fn try_downgrade_map( + this: Self, + f: F, + ) -> Result, Self> + where + F: FnOnce(&T) -> Option<&U>, + { + let data = match f(&*this) { + Some(data) => data as *const U, + None => return Err(this), + }; + let this = this.skip_drop(); + let guard = OwnedRwLockReadGuard { + lock: this.lock, + data, + _p: PhantomData, + #[cfg(all(tokio_unstable, feature = "tracing"))] + resource_span: this.resource_span, + }; + + // Release all but one of the permits held by the write guard + let to_release = (this.permits_acquired - 1) as usize; + guard.lock.s.release(to_release); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + write_locked = false, + write_locked.op = "override", + ) + }); + + #[cfg(all(tokio_unstable, feature = "tracing"))] + guard.resource_span.in_scope(|| { + tracing::trace!( + target: "runtime::resource::state_update", + current_readers = 1, + current_readers.op = "add", + ) + }); + + Ok(guard) + } + /// Converts this `OwnedRwLockWriteGuard` into an /// `OwnedRwLockMappedWriteGuard`. This method can be used to store a /// non-mapped guard in a struct field that expects a mapped guard. From cd5e3cb3615025ab531d85eb68324d12296f9c62 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Tue, 7 Mar 2023 19:39:40 +0000 Subject: [PATCH 4/5] Improved some doctests readability. --- tokio/src/sync/rwlock/owned_write_guard.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tokio/src/sync/rwlock/owned_write_guard.rs b/tokio/src/sync/rwlock/owned_write_guard.rs index a8d671e9781..a8ce4a1603f 100644 --- a/tokio/src/sync/rwlock/owned_write_guard.rs +++ b/tokio/src/sync/rwlock/owned_write_guard.rs @@ -125,7 +125,8 @@ impl OwnedRwLockWriteGuard { /// # async fn main() { /// let lock = Arc::new(RwLock::new(Foo(1))); /// - /// let mapped = OwnedRwLockWriteGuard::downgrade_map(Arc::clone(&lock).write_owned().await, |f| &f.0); + /// let guard = Arc::clone(&lock).write_owned().await; + /// let mapped = OwnedRwLockWriteGuard::downgrade_map(guard, |f| &f.0); /// let foo = lock.read_owned().await; /// assert_eq!(foo.0, *mapped); /// # } @@ -258,7 +259,8 @@ impl OwnedRwLockWriteGuard { /// # async fn main() { /// let lock = Arc::new(RwLock::new(Foo(1))); /// - /// let guard = OwnedRwLockWriteGuard::try_downgrade_map(Arc::clone(&lock).write_owned().await, |f| Some(&f.0)).expect("should not fail"); + /// let guard = Arc::clone(&lock).write_owned().await; + /// let guard = OwnedRwLockWriteGuard::try_downgrade_map(guard, |f| Some(&f.0)).expect("should not fail"); /// let foo = lock.read_owned().await; /// assert_eq!(foo.0, *guard); /// # } From f9f2930fcdce8b832e0baba1c1efe03c43675410 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Tue, 7 Mar 2023 19:57:05 +0000 Subject: [PATCH 5/5] Added tests for `{Mapped}RwLockWriteGuard::{downgrade_map, try_downgrade_map}`. --- tokio/tests/sync_rwlock.rs | 52 +++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/tokio/tests/sync_rwlock.rs b/tokio/tests/sync_rwlock.rs index 948ec131ed1..667f621721f 100644 --- a/tokio/tests/sync_rwlock.rs +++ b/tokio/tests/sync_rwlock.rs @@ -13,7 +13,7 @@ use std::task::Poll; use futures::future::FutureExt; -use tokio::sync::RwLock; +use tokio::sync::{RwLock, RwLockWriteGuard}; use tokio_test::task::spawn; use tokio_test::{assert_pending, assert_ready}; @@ -279,3 +279,53 @@ fn try_read_try_write() { assert_eq!(*lock.try_read().unwrap(), 1515); } + +#[maybe_tokio_test] +async fn downgrade_map() { + let lock = RwLock::new(0); + let write_guard = lock.write().await; + let mut read_t = spawn(lock.read()); + + // We can't create a read when a write exists + assert_pending!(read_t.poll()); + + // During the call to `f`, `read_t` doesn't have access yet. + let read_guard1 = RwLockWriteGuard::downgrade_map(write_guard, |v| { + assert_pending!(read_t.poll()); + v + }); + + // After the downgrade, `read_t` got the lock + let read_guard2 = assert_ready!(read_t.poll()); + + // Ensure they're equal, as we return the original value + assert_eq!(&*read_guard1 as *const _, &*read_guard2 as *const _); +} + +#[maybe_tokio_test] +async fn try_downgrade_map() { + let lock = RwLock::new(0); + let write_guard = lock.write().await; + let mut read_t = spawn(lock.read()); + + // We can't create a read when a write exists + assert_pending!(read_t.poll()); + + // During the call to `f`, `read_t` doesn't have access yet. + let write_guard = RwLockWriteGuard::try_downgrade_map(write_guard, |_| { + assert_pending!(read_t.poll()); + None::<&()> + }) + .expect_err("downgrade didn't fail"); + + // After `f` returns `None`, `read_t` doesn't have access + assert_pending!(read_t.poll()); + + // After `f` returns `Some`, `read_t` does have access + let read_guard1 = RwLockWriteGuard::try_downgrade_map(write_guard, |v| Some(v)) + .expect("downgrade didn't succeed"); + let read_guard2 = assert_ready!(read_t.poll()); + + // Ensure they're equal, as we return the original value + assert_eq!(&*read_guard1 as *const _, &*read_guard2 as *const _); +}