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.

[ds84182]

Additionally, pthread_rwlock_t, which before #95801 backed the RwLock implementation on Linux, explicitly allows reentrant read locking on the same thread.

https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_rwlock_rdlock.html

A thread may hold multiple concurrent read locks on rwlock (that is, successfully call the pthread_rwlock_rdlock() function n times). If so, the thread must perform matching unlocks (that is, it must call the pthread_rwlock_unlock() function n times).

[hzeller]

Of course it is possible to have multiple read locks on the same thread, but it is not a semantically valid situation as soon as there are more threads involved in which others may attempt to .write() (and why would we use locks if we only do one thread). This can and will deadlock for most underlying implementations if the reads and writes are interleaved like in the deadlock example.

Not panic, but there is no guarantee that we can give that deadlocks can not happen on all implementations (in fact any implementation that would provide such guarantees, would required to have a very expensive overhead in book-keeping, which I doubt any of them do...)

Therefore I suggest that we reformulate the example to not create the deadlock-prone situation (e.g. by illustrating two different threads that have a read lock out with one that writes).
People will copy-paste code and think it is a sound example when it isn't. We should not lead users astray.
(also, the deadlock example should probably not be hidden in a 'Detail' block, but very prominently visible)

[ds84182]

it is not a semantically valid situation as soon as there are more threads involved in which others may attempt to .write()

This can and will deadlock for most underlying implementations

Is there proof of this? As far as I'm aware, attempting to obtain a write lock does not block readers if there are other concurrent readers. Which therefore means reentrant read locks do not lead to deadlocks in any scenario.

[gmorenz]

Is there proof of this? As far as I'm aware, attempting to obtain a write lock does not block readers if there are other concurrent readers. Which therefore means reentrant read locks do not lead to deadlocks in any scenario.

Yes, this program will deadlock on linux compiled with a recent rustc with no optimizations after writing ~50k lines to stdout.

use std::sync::{Arc, RwLock};

fn main() {
    let l = Arc::new(RwLock::new(()));
    for i in 0.. i32::MAX {
        println!("{i}");
        let l2 = l.clone();
        std::thread::spawn(move || {
            let g1 = l2.read();
            let g2 = l2.read();
        });
        l.write();
    }
}

What happens is the reading thread acquires a read lock, the writing thread attempts to acquire a write lock and blocks waiting for the first read lock to be released, the reading thread attempts to acquire a second readlock and blocks waiting for the write lock to have its turn and thus never releases the first read lock resulting in a deadlock.

Though the claim that this occurs as soon as there are more threads involved in which others attempt to .write() overstates it slightly, that's only true if the other threads aren't synchronized. There is nothing stopping you from using independent synchronization methods to avoid a deadlock and using the RwLock merely to guarantee memory safety if you made a mistake with the other synchronization.

[ds84182]

Ah I forgot the variable bindings in my test. Been writing too much C lately.

I think it would be reasonable to clarify that a deadlock detection panic is possible when debug assertions are enabled (either done by std or underlying platform), and otherwise a reentrant read lock's behavior is platform-specific and may change across Rust versions. Without debug assertions, implementations may deadlock or fall back to a different locking strategy. If a particular strategy is desired, developers can rely upon a crate like parking_lot or platform-specific locking primitives. Developers cannot rely on deadlocking for correctness.