Add RwLockWriteGuard::{downgrade_map, try_downgrade_map}.

[Zenithsiz]

Adds RwLockWriteGuard::{downgrade_map, try_downgrade_map}.

Motivation

These functions allow the following code to work:

impl T {
    fn try_get(&self) -> Option<&U> { ... }
}

pub async fn foo(lock: RwLock<T>) -> RwLockReadGuard<U> {
    // Check with a read lock
    if let Ok(value) = RwLockReadGuard::try_map(lock.read().await, |value| value.try_get()) {
        return value;
    }

    // Else "upgrade" and check if it was changed in the meantime
    let guard = lock.write().await;
    if let Ok(value) = RwLockWriteGuard::try_downgrade_map(guard, |value| value.try_get()) {
        return value;
    }

    // Use `guard` mutably
    // ...
}

The code uses a double-checked lock on a RwLock to do some action, and then returns a mapped read guard to the inner data.

With the current suite of functions, we can't easily perform the second check efficiently. We need to not unlock the rwlock, which means we'd need to first call try_get to check if it's available, then perform a downgrade and map it with try_get().unwrap() to get the value again. This involves calling try_get twice, which isn't desirable.

These function are Fn(&T) -> &U, so I don't believe there are any soundness issues (I made a stack overflow question to check if it was sound, and someone suggested that if it was, this could be a good API to add, so I made this PR).

Solution

We simply add the functions on RwLockWriteGuard. RwLockMappedWriteGuard can't have them, due to unsoundness, but a non-mapped RwLockWriteGuard is fine.

[Darksonn]

Can you please add a justification to these methods for why they are sound?

[Zenithsiz]

I'm fairly certain they are safe, but I'm not 100% sure. Originally I was going to open an issue asking if it was safe, given the surrounding API and, if so, if they could be added to tokio. But since the implementation was relatively easy, I just decided to open up a PR and I was hoping we could discuss here the safety.

In detail:
The issues in this issue are due to functions that support interior mutability usually containing an fn(&mut Wrapper<T>) -> &mut T to "circumvent" the interior mutability. However, when downgrading, you could then keep a &T, and then proceed to call a function on &Wrapper<T> that changes the T, thus breaking the promise that &T is noalias (assuming T doesn't contain an unsafe cell). The example in the linked issue illustrates this well with RwLock<Cell<u32>>.

However, with {try_}downgrade_map, we only give out a &Wrapper<T>, so you can't "circumvent" the interior mutability and get a &T, I believe. I can't see any way obvious way to break this, but I just wanted to discuss it since there have been issues with downgrading mapped locks.

If we come to the conclusion that it isn't, should I just add the justification to the inside of the functions with a SAFETY: ... comment?

[Darksonn]

Here's one argument for their soundness:

1. If the closure fails, then you've just accessed the contents immutably, which is fine given a write guard.
2. If the closure succeeds, then your code is equivalent to using downgrade followed by mapping the resulting read guard.

@Amanieu Do you have any feedback for the soundness of the proposed methods in this PR?

[Amanieu]

I believe this is sound.

[satakuma]

I think @Darksonn's argument about soundness is convincing.

If we decide that these methods should be added, I think it also makes sense to add them to the OwnedRwLockWriteGuard.

[satakuma]

You should also release the write access here. Currently it's not possible for another reader to access the guarded data after using one of these functions.

Releasing the write access should be done by releasing all permits but one to the backing semaphore. You can take a look how downgrade is implemented.

downgrade also records some tracing events. It would be great to have them in both of these functions as well.

[Darksonn]

You will need to add tests for this. In particular, you should make sure to test that its possible to acquire another read lock while holding the downgraded read lock.

[Zenithsiz]

The doctests check if a read lock can be acquired after the {try_}downgrade_map, but is this enough? Would you prefer a proper test on tests/ instead?

[satakuma]

I think this change looks good, I left some comments inline.

Would you prefer a proper test on tests/ instead?

I think doctests are fine.

[satakuma]

Could you split this line? It's hard to see what's going on here from the docs page.

Suggested change

	/// 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);

[satakuma]

The same here.

[satakuma]

It would be great to have a test which checks this guarantee. For example, a test which checks if a reader which was already waiting for the access before calling this function is still waiting after this function returns Err.

[Zenithsiz]

I think the tests are exhaustive enough for all scenarios. Let me know if there any cases I've missed.

[satakuma]

Looks good to me! I'll let @Darksonn take a look as well.

[Darksonn]

LGTM.