Ambiguity in RwLock documentation about multiple .read() calls on the same thread

[durin42]

Location (URL)

https://doc.rust-lang.org/std/sync/struct.RwLock.html#method.read

Summary

We're exploring backing RwLock with primitives from https://abseil.io/docs/cpp/guides/synchronization and encountered a surprise: RwLock is not documentedly reentrant for read lock holders, but there's an example that would fail if it wasn't. On the other hand, the documented "Potential deadlock example" is related. Then in the docs for fn read we see "This function might panic when called if the lock is already held by the current thread."

I think this means that it's not safe to call .read() on an RwLock multiple times in the same thread - but then there's this test in crossbeam-utils that fails on line 16 when the abseil deadlock detector is active, which is correct if the RwLock isn't reentrant.

Should we revise the documentation to clarify that RwLock isn't reentrant, since that's what's already implied? Or do we need to treat the example from crossbeam-utils as evidence that RwLock needs to be permissive in this regard even though it's a potential bug-in-waiting based on the deadlock example?

In favor of revising to clarify you shouldn't call .read() more than once per thread:

• leaves more flexibility for implementations delegating to OS-level constructs
• probably lets impls be more efficient
• lines up clearly with the potential deadlock example in the docs and makes adding deadlock detection more viable

In favor of clarifying to allow .read() more than once per thread:

• A little easier to use for users
• Hyrum's Law on the test in crossbeam-utils, which might be the tip of an iceberg

I'd be happy to send a doc update that makes the example use multiple threads rather than calling .read() (or add a comment describing how they should be in threads) and adds some warning text to .read() about only holding one reader lock per thread, but I wanted to clarify what direction we should go before doing so.

So: what's the consensus?

[tmandry]

The issue seems to be that the RwLocks docs are unclear and contradict themselves:

First, you have the read method docs saying

This function might panic when called if the lock is already held by the current thread.

This straightforwardly implies that you shouldn't call read twice on the same thread without dropping the lock in between. But I suppose there's a chance it could be meant to say "if the write lock is already held by the current thread".

Second, you have the potential deadlock example (hidden behind a collapse) that would certainly imply the pattern of holding two read locks on the same thread is very risky, because it can lead to deadlocks:

// Thread 1              |  // Thread 2
let _rg1 = lock.read();  |
                         |  // will block
                         |  let _wg = lock.write();
// may deadlock          |
let _rg2 = lock.read();  |

But then third, most prominently near the top of the docs, we have an example doing just the thing that we advised against.

Example

use std::sync::RwLock;

let lock = RwLock::new(5);

// many reader locks can be held at once
{
    let r1 = lock.read().unwrap();
    let r2 = lock.read().unwrap();
    assert_eq!(*r1, 5);
    assert_eq!(*r2, 5);
} // read locks are dropped at this point

// only one write lock may be held, however
{
    let mut w = lock.write().unwrap();
    *w += 1;
    assert_eq!(*w, 6);
} // write lock is dropped here

To me the example seems broken. When I saw it first on reading the docs I thought it was clearly a reentrant lock and you could hold two read locks just fine, but the rest of the docs don't agree with that.

The other question that needs to be answered here is what is the range of acceptable behaviors for an implementation of these locks. In other words, if you acquire two read locks on the same thread, is it always correct to (1) deadlock, (2) panic, (3) abort with an error message?

[gmorenz]

I've always been under the impression that I can use RwLock as a Sync RefCell, and this is supported by the docs saying "The corresponding Sync version of RefCell is RwLock". Which is to say that I've been under the impression that it is guaranteed that two reads on the same thread do not panic so long as there is no possible deadlock (there is no simultaneous write on any thread).

This is consistent with the current behaviour. Consistent with both examples in the current documentation (that tmandry cites above). And consistent with the statement that "This function might panic when called if the lock is already held by the current thread" provided you read it as "this function might [in certain conditions, specifically when deadlock is possible] panic when called if the lock is already held by the current thread" and not "this function might panic when called if the lock is already held by the current thread [and implementations are permitted to do so unconditionally]".

I don't actually think the second reading is a valid one, or that the documentation is either contradictory or incorrect here. This is documentation for users of the API, not for implementors of new RwLocks, and it's true as a user that it might panic. That doesn't mean that an implementation that panics under all circumstances is correct.

The documentation could be cleaned up to clarify the guarantee though...

Edit: Also consistent with the crossbeam tests. All read locks are always dropped prior to any write locks - so no deadlock is possible even without (fully) reentrant locks.