Add RwLockReadGuard::map for transforming guards to contain sub-borrows. #30834

reem · 2016-01-12T03:14:12Z

This is very useful when the RwLock is synchronizing access to a data
structure and you would like to return or store guards which contain
references to data inside the data structure instead of the data structure
itself.

rust-highfive · 2016-01-12T03:14:24Z

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

reem · 2016-01-12T03:14:50Z

I've opened this without an accompanying RFC since it's a simple, unstable API. Let me know if adding this as unstable would require an RFC and I can write one up.

reem · 2016-01-12T03:16:34Z

For reference, my current use case is a type which contains a RwLock<HashMap<K, V>> and I have an API where I would like to return RwLockReadGuard<V>, which is currently impossible. Without this API, I would have to define a new type and look up the value on every dereference of that type.

huonw · 2016-01-12T04:42:06Z

I would think these APIs should match the similar APIs on the RefCell guards, e.g. http://doc.rust-lang.org/std/cell/struct.Ref.html has map and filter_map.

NB. those have the functionality as associated functions, not methods: we try to avoid methods on generic smart-pointers/Deref types because they interfere with method calls to the internal one. The method form would actually be a breaking change, since, for instance, if x: RwLock<Option<...>> then let y = x.read(); let z: Option<()> = y.map(|_| ()) compiles, but this PR would make z try to be assigned an RwLockReadGuard<()>.

Also, MutexGuard and RwLockWriteGuard could probably have them.

(I'm in favour of this.)

reem · 2016-01-12T04:45:22Z

Note: I have prepped a temporary fork of RwLock to get this API on stable for my personal use which I'll keep around until these changes become stabilized. (We have a rust in prod usecase that depends on this feature and we would like to compile on stable)

Agreed with all of @huonw's comments, will update.

sfackler · 2016-01-12T05:38:38Z

If we're going to add this (which I have no real problem with), we should also stick this on MutexGuard and RwLockWriteGuard in this PR.

reem · 2016-01-12T07:46:21Z

Can do.

reem · 2016-01-12T21:18:36Z

Hmm, I am looking at this code again and I think there is a subtle bug - the original guard should be mem::forgeted so as to avoid prematurely releasing the read lock.

rphmeier · 2016-01-21T14:46:56Z

I think this is a definite improvement to the RwLock API, having found myself wanting something like this in my own code and being forced to approximate with unsafe code. I also agree with @sfackler that this should be added to the guards of various synchronization primitives, although the situation gets slightly hairier with RwLockWriteGuard and MutexGuard, since you have transformations from both &mut T -> &mut U as well as &mut T -> &U.

In particular, I wonder whether it is possible or makes sense to actually downgrade an RwLockWriteGuard to an RwLockReadGuard in the &mut T -> &U case, since it seems like overkill to continue to hold exclusive access if no further mutation is going to occur for the duration of the borrow.

sfackler · 2016-01-21T17:52:00Z

@rphmeier pthread rwlocks don't support a downgrade operation, unfortunately, so we wouldn't be able to support a &mut T -> &U method on unix-y platforms.

rphmeier · 2016-01-22T03:46:28Z

@sfackler Ah, that's unfortunate. At the very least, &mut T -> &U transformations can still be supported, they'll just have to continue holding exclusive access via a RwLockWriteGuard.

arthurprs · 2016-01-23T13:03:07Z

This is very cool, I wanted something similar while writing floki.

sfackler · 2016-01-29T07:06:31Z

I believe we're good to go with this if the same method gets added to MutexGuard, right @alexcrichton?

alexcrichton · 2016-01-29T07:28:29Z

Ah yes sorry I meant to get to this sooner. We discussed this in the libs meeting and decided to move forward. We'd like to add this to the following types, though:

MutexGuard
RwLockReadGuard
RwLockWriteGuard

Also feel free to hook this up to this tracking issue: #27746

reem · 2016-01-30T00:18:52Z

Updated the PR to include the method as an associated function for RwLockReadGuard, RwLockWriteGuard and MutexGuard, also added specific tests of poisoning behavior for RwLockWriteGuard and MutexGuard, where they are relevant.

reem · 2016-01-30T00:19:26Z

I currently added the methods as unstable, but could change the PR to land them as insta-stable if that is desirable.

sfackler · 2016-01-30T00:20:15Z

I think we'll land them as unstable but stabilize at the same time as RefCellGuard::map.

reem · 2016-01-30T00:26:03Z

Ok, I changed them to use a common feature guard_map.

alexcrichton · 2016-01-30T00:59:06Z

src/libstd/sync/rwlock.rs

+               reason = "recently added, needs RFC for stabilization",
+               issue = "0")]
+    pub fn map<U: ?Sized, F>(this: Self, cb: F) -> RwLockReadGuard<'rwlock, U>
+    where F: FnOnce(&'rwlock T) -> &'rwlock U {


Stylistically where clauses like this in the standard library tend to be formatted like:

pub fn map<F>(...) -> T where F: ... { // ... }

This is very useful when the lock is synchronizing access to a data structure and you would like to return or store guards which contain references to data inside the data structure instead of the data structure itself.

alexcrichton · 2016-01-30T01:02:29Z

src/libstd/sync/rwlock.rs

+    where F: FnOnce(&'rwlock mut T) -> &'rwlock mut U {
+        let new_data = unsafe {
+            let data: &'rwlock mut T = &mut *this.__data.get();
+            mem::transmute::<&'rwlock mut U, &'rwlock UnsafeCell<U>>(cb(data))


The strategy above of changing the stored type to just T seemed to work out well, could that be done here to avoid the transmute?

I'll change both RwLockWriteGuard and MutexGuard to store &mut Ts.

reem · 2016-01-30T21:30:09Z

Ok, I think this should be ready to land now.

huonw · 2016-01-31T00:13:36Z

src/libstd/sync/mutex.rs

+    /// ```
+    #[unstable(feature = "guard_map",
+               reason = "recently added, needs RFC for stabilization",
+               issue = "0")]


An issue should be opened and these numbers updated before this lands.

We're going to fold this into #27746 so that's the issue number to use here.

reem · 2016-01-31T00:42:44Z

Added the issue number and fixed the failing doctests, hopefully passes this time.

alexcrichton · 2016-01-31T18:05:50Z

src/libstd/sync/mutex.rs

@@ -231,7 +231,7 @@ impl<T: ?Sized> Mutex<T> {
    #[stable(feature = "rust1", since = "1.0.0")]
    pub fn try_lock(&self) -> TryLockResult<MutexGuard<T>> {
        if unsafe { self.inner.lock.try_lock() } {
-            Ok(try!(MutexGuard::new(&*self.inner, &self.data)))
+            Ok(try!(unsafe { MutexGuard::new(&*self.inner, &self.data) }))


With this new proliferation of unsafe blocks, can blocks like this just be merged with the one above? (e.g. have one surrounding block)

reem · 2016-02-02T03:09:57Z

@alexcrichton updated for style changes both in mutex and rwlock

alexcrichton · 2016-02-02T03:43:50Z

src/libstd/sync/rwlock.rs

@@ -178,7 +178,7 @@ impl<T: ?Sized> RwLock<T> {
    #[stable(feature = "rust1", since = "1.0.0")]
    pub fn read(&self) -> LockResult<RwLockReadGuard<T>> {
        unsafe { self.inner.lock.read() }
-        RwLockReadGuard::new(&*self.inner, &self.data)
+        unsafe { RwLockReadGuard::new(&*self.inner, &self.data) }


Ah looks like the rwlock here needs similar treatment as a mutex

reem · 2016-02-03T02:17:15Z

How would we feel about also adding Ref::filter_map equivalents?

EDIT: Also updated for last nits.

alexcrichton · 2016-02-03T17:32:45Z

In theory we're slating Ref::filter_map for deprecation so it's fine to not add those as part of this PR.

@bors: r+ fc875b0

Thanks!

This is very useful when the RwLock is synchronizing access to a data structure and you would like to return or store guards which contain references to data inside the data structure instead of the data structure itself.

bors · 2016-02-03T19:26:06Z

⌛ Testing commit fc875b0 with merge 18c1781...

bors · 2016-02-03T22:40:30Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

reem · 2016-02-05T10:17:09Z

@alexcrichton damn, I've discovered a safety hole in MutexGuard::map - it is not safe in combination with Condvar, since the "inner" borrow can be invalidated by another thread taking the lock during a Condvar::wait see: http://is.gd/RM038X.

reem · 2016-02-05T10:17:43Z

This is not a problem with the RwLock methods, since the RwLock guards cannot be used with a Condvar.

reem · 2016-02-05T10:18:33Z

It would be possible to provide a similar, but safe, API by defining a separate type for "mapped" mutex guards that can't be used with a Condvar.

alexcrichton · 2016-02-05T17:29:38Z

Nice catch @reem! Certainly an interesting interaction between the two APIs...

rust-highfive assigned alexcrichton Jan 12, 2016

reem force-pushed the rwlock-read-guard-map branch 2 times, most recently from 14039a1 to d18ccce Compare January 12, 2016 04:05

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 12, 2016

alexcrichton removed the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 29, 2016

reem force-pushed the rwlock-read-guard-map branch 2 times, most recently from 876f311 to 4385db1 Compare January 30, 2016 00:15

alexcrichton reviewed Jan 30, 2016
View reviewed changes

reem force-pushed the rwlock-read-guard-map branch from 4385db1 to a4343e9 Compare January 30, 2016 01:00

alexcrichton reviewed Jan 30, 2016
View reviewed changes

reem force-pushed the rwlock-read-guard-map branch from 93d78ab to bf60078 Compare January 30, 2016 21:29

huonw reviewed Jan 31, 2016
View reviewed changes

reem force-pushed the rwlock-read-guard-map branch from 64aeb56 to 6803736 Compare January 31, 2016 04:51

alexcrichton reviewed Jan 31, 2016
View reviewed changes

reem force-pushed the rwlock-read-guard-map branch from 6803736 to 4f61979 Compare February 2, 2016 03:09

reem force-pushed the rwlock-read-guard-map branch from 4f61979 to 7292fbc Compare February 2, 2016 03:14

alexcrichton reviewed Feb 2, 2016
View reviewed changes

Add issue number to guard map methods.

fc875b0

reem force-pushed the rwlock-read-guard-map branch from 7292fbc to fc875b0 Compare February 3, 2016 00:34

bors merged commit fc875b0 into rust-lang:master Feb 3, 2016

reem deleted the rwlock-read-guard-map branch February 5, 2016 10:16

reem mentioned this pull request Feb 5, 2016

Tracking issue for cell_extras stabilization #27746

Closed

sfackler mentioned this pull request Aug 30, 2017

{MutexGuard, RwLockReadGuard, RwLockWriteGuard}::map #44189

Closed

Add RwLockReadGuard::map for transforming guards to contain sub-borrows. #30834

Add RwLockReadGuard::map for transforming guards to contain sub-borrows. #30834

Conversation

reem commented Jan 12, 2016

rust-highfive commented Jan 12, 2016

reem commented Jan 12, 2016

reem commented Jan 12, 2016

huonw commented Jan 12, 2016

reem commented Jan 12, 2016

sfackler commented Jan 12, 2016

reem commented Jan 12, 2016

reem commented Jan 12, 2016

rphmeier commented Jan 21, 2016

sfackler commented Jan 21, 2016

rphmeier commented Jan 22, 2016

arthurprs commented Jan 23, 2016

sfackler commented Jan 29, 2016

alexcrichton commented Jan 29, 2016

reem commented Jan 30, 2016

reem commented Jan 30, 2016

sfackler commented Jan 30, 2016

reem commented Jan 30, 2016

alexcrichton Jan 30, 2016

Choose a reason for hiding this comment

alexcrichton Jan 30, 2016

Choose a reason for hiding this comment

reem Jan 30, 2016

Choose a reason for hiding this comment

reem commented Jan 30, 2016

huonw Jan 31, 2016

Choose a reason for hiding this comment

sfackler Jan 31, 2016

Choose a reason for hiding this comment

reem commented Jan 31, 2016

alexcrichton Jan 31, 2016

Choose a reason for hiding this comment

reem commented Feb 2, 2016

alexcrichton Feb 2, 2016

Choose a reason for hiding this comment

reem commented Feb 3, 2016

alexcrichton commented Feb 3, 2016

bors commented Feb 3, 2016

bors commented Feb 3, 2016

reem commented Feb 5, 2016

reem commented Feb 5, 2016

reem commented Feb 5, 2016

alexcrichton commented Feb 5, 2016