rust-lang / rust-lang/rust

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

Open
#149,693 7 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-docs needs-triage T-libs
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

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?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read the RwLock::read documentation and its linked examples, then compare them with the potential-deadlock example. Review the linked crossbeam-utils sharded_lock test as evidence of current behavior. The issue needs consensus on reentrancy before a documentation change can be considered done.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
operating-systems
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.