Skip to content

Clarified docs in std::sync::RwLock + added test to ensure that max reader count is respected #153555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asder8215 wants to merge 1 commit into
base: main
Choose a base branch
from
+65 −2
Open

Clarified docs in std::sync::RwLock + added test to ensure that max reader count is respected #153555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/std/src/sync/poison/rwlock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::sys::sync as sys;
///
/// In comparison, a [`Mutex`] does not distinguish between readers or writers
/// that acquire the lock, therefore blocking any threads waiting for the lock to
/// become available. An `RwLock` will allow any number of readers to acquire the
/// become available. An `RwLock` will allow multiple readers to acquire the
/// lock as long as a writer is not holding the lock.
///
/// The priority policy of the lock is dependent on the underlying operating
Expand Down
65 changes: 64 additions & 1 deletion library/std/tests/sync/rwlock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::sync::{
Arc, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard, RwLockWriteGuard,
TryLockError,
};
use std::{hint, mem, thread};
use std::{hint, mem, thread, u32};

use rand::Rng;

Expand Down Expand Up @@ -883,3 +883,66 @@ fn test_rwlock_with_mut() {
assert_eq!(*rwlock.read(), 5);
assert_eq!(result, 10);
}

// To note: there are (currently) four different implementations of Rwlock:
// - On Windows (but not Win 7), Linux, Android, FreeBSD, OpenBSD, DragonFly,
// Fuchsia, WASM, Hermit, and Motor OSs, it relies on rwlock/futex.rs, which has
// a max reader of 1 << 30 - 2 (or 1073741822). A "too many active reader" error
// is displayed after it exceeds the max number of readers.
// - On Unix, Win 7, Fortranix (target env of sgx), Xous, and TeeOS, it leans
// on rwlock/queue.rs, which uses a linked list under the hood stored on the stack
// to hold a queue of waiters. Assuming no stack overflow, the max number of readers
// that can be queued up at one time is limited to usize::MAX - (1 << 4) as the first
// four bits are reserved for LOCKED, QUEUED, QUEUE_LOCKED, and DOWNGRADED flags. Any
// pending readers after that max count, parks the thread and tries to acquire a read lock
// again when the thread wakes up.
// - On SolidASP3, it leans on rwlock/solid.rs, which utilizes rwl_loc_rdl, so the max
// number of readers depends on the internal implementation of rwl_loc_rdl.
// - Every other platforms utilizes rwlock/no_threads.rs, which has a max reader of
// isize::MAX. An arithmetic overflow error occurs if it exceeds the max reader count.
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this arithmetic overflow look like? A panic?

I think we want to know if there are new targets added that don't meet the target (MAX_READERS here). I'd rather see the cfg on the MAX_READERS constant and we can lower it / set it to different values depending on the cfg.

E.g.:

const MAX_READERS: if cfg!(...) { ... } else if cfg!(...) { ... } else { ... };

Copy link
Copy Markdown
Contributor Author

@asder8215 asder8215 Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this arithmetic overflow look like? A panic?

There's no explicit panic in the arithmetic overflow for the no_threads RwLock impl. If we take a look at the code inside library/std/src/sys/sync/rwlock/no_thread.rs:

use crate::cell::Cell;

pub struct RwLock {
    // This platform has no threads, so we can use a Cell here.
    mode: Cell<isize>,
}

unsafe impl Send for RwLock {}
unsafe impl Sync for RwLock {} // no threads on this platform

impl RwLock {
    #[inline]
    pub const fn new() -> RwLock {
        RwLock { mode: Cell::new(0) }
    }

    #[inline]
    pub fn read(&self) {
        let m = self.mode.get();
        if m >= 0 {
            self.mode.set(m + 1);
        } else {
            rtabort!("rwlock locked for writing");
        }
    }

    #[inline]
    pub fn try_read(&self) -> bool {
        let m = self.mode.get();
        if m >= 0 {
            self.mode.set(m + 1);
            true
        } else {
            false
        }
    }

    #[inline]
    pub fn write(&self) {
        if self.mode.replace(-1) != 0 {
            rtabort!("rwlock locked for reading")
        }
    }

    #[inline]
    pub fn try_write(&self) -> bool {
        if self.mode.get() == 0 {
            self.mode.set(-1);
            true
        } else {
            false
        }
    }

    #[inline]
    pub unsafe fn read_unlock(&self) {
        self.mode.set(self.mode.get() - 1);
    }

    #[inline]
    pub unsafe fn write_unlock(&self) {
        assert_eq!(self.mode.replace(0), -1);
    }

    #[inline]
    pub unsafe fn downgrade(&self) {
        assert_eq!(self.mode.replace(1), -1);
    }
}

There is overflow that could occur on mode when we reach isize::MAX readers and decide to do another RwLock::read() call here (though this could end up being silent).

I have a different PR as well addressing what I find odd about the RwLock::write here with always replacing the mode value with -1 even when it's reader locked.

Should there be an assertion here that does something similar to rwlock/futex.rs with erroring with the message "too many active read locks on RwLock" when we go beyond the max reader count? I'm not sure how to test this locally or if CI would be able to run the max reader test we have here on the no_threads impl to see if it works as intended in.

I think we want to know if there are new targets added that don't meet the target (MAX_READERS here). I'd rather see the cfg on the MAX_READERS constant and we can lower it / set it to different values depending on the cfg.
E.g.:
const MAX_READERS: if cfg!(...) { ... } else if cfg!(...) { ... } else { ... };

I can do this. I'm a bit concerned about the rwlock/queue.rs implementation because technically the max readers is usize::MAX - (1 << 4) from my understanding of this piece of code:

/// Marks the state as read-locked, if possible.
#[inline]
fn read_lock(state: State) -> Option<State> {
    if state.addr() & QUEUED == 0 && state.addr() != LOCKED {
        Some(without_provenance_mut(state.addr().checked_add(SINGLE)? | LOCKED))
    } else {
        None
    }
}

However, at least from my understanding of RwLock::lock_contended():

#[cold]
    fn lock_contended(&self, write: bool) {
        let mut node = Node::new(write);
        let mut state = self.state.load(Relaxed);
        let mut count = 0;
        let update_fn = if write { write_lock } else { read_lock };

        loop {
            // Optimistically update the state.
            if let Some(next) = update_fn(state) {
                // The lock is available, try locking it.
                match self.state.compare_exchange_weak(state, next, Acquire, Relaxed) {
                    Ok(_) => return,
                    Err(new) => state = new,
                }
                continue;
            } else if state.addr() & QUEUED == 0 && count < SPIN_COUNT {
                // If the lock is not available and no threads are queued, optimistically spin for a
                // while, using exponential backoff to decrease cache contention.
                for _ in 0..(1 << count) {
                    spin_loop();
                }
                state = self.state.load(Relaxed);
                count += 1;
                continue;
            }
            // The optimistic paths did not succeed, so fall back to parking the thread.

            // First, prepare the node.
            node.prepare();

            // If there are threads queued, this will set the `next` field to be a pointer to the
            // first node in the queue.
            // If the state is read-locked, this will set `next` to the lock count.
            // If it is write-locked, it will set `next` to zero.
            node.next.0 = AtomicPtr::new(state.mask(NODE_MASK).cast());
            node.prev = AtomicLink::new(None);

            // Set the `QUEUED` bit and preserve the `LOCKED` and `DOWNGRADED` bit.
            let mut next = ptr::from_ref(&node)
                .map_addr(|addr| addr | QUEUED | (state.addr() & (DOWNGRADED | LOCKED)))
                as State;

            let mut is_queue_locked = false;
            if state.addr() & QUEUED == 0 {
                // If this is the first node in the queue, set the `tail` field to the node itself
                // to ensure there is a valid `tail` field in the queue (Invariants 1 & 2).
                // This needs to use `set` to avoid invalidating the new pointer.
                node.tail.set(Some(NonNull::from(&node)));
            } else {
                // Otherwise, the tail of the queue is not known.
                node.tail.set(None);

                // Try locking the queue to eagerly add backlinks.
                next = next.map_addr(|addr| addr | QUEUE_LOCKED);

                // Track if we changed the `QUEUE_LOCKED` bit from off to on.
                is_queue_locked = state.addr() & QUEUE_LOCKED == 0;
            }

            // Register the node, using release ordering to propagate our changes to the waking
            // thread.
            if let Err(new) = self.state.compare_exchange_weak(state, next, AcqRel, Relaxed) {
                // The state has changed, just try again.
                state = new;
                continue;
            }
            // The node has been registered, so the structure must not be mutably accessed or
            // destroyed while other threads may be accessing it.

            // Guard against unwinds using a `PanicGuard` that aborts when dropped.
            let guard = PanicGuard;

            // If the current thread locked the queue, unlock it to eagerly adding backlinks.
            if is_queue_locked {
                // SAFETY: This thread set the `QUEUE_LOCKED` bit above.
                unsafe {
                    self.unlock_queue(next);
                }
            }

            // Wait until the node is removed from the queue.
            // SAFETY: the node was created by the current thread.
            unsafe {
                node.wait();
            }

            // The node was removed from the queue, disarm the guard.
            mem::forget(guard);

            // Reload the state and try again.
            state = self.state.load(Relaxed);
            count = 0;
        }
    }

If we receive None from read_lock() or reach that possible max reader count, the pending reader will try to acquire the lock again another time.

I guess my questions for this scenario are:

  • Should I just set MAX_READER to usize::MAX - (1 << 4) for targets associated with rwlock/queue.rs? (this will make the test run slower on 64 bit machines though, wouldn't it?)
  • If we set MAX_READER to usize::MAX - (1 << 4), would it be possible for this to stack overflow as all allocation for this RwLock impl is stack allocated?

The last question I have is I'm aware that for solid_asp3 it RwLock::read() depends on how rwl_loc_rdl is internally implemented. That's my understanding at least from looking at std/src/sys/pal/solid/abi/mod.rs:

...
// `rwlock.h`
unsafe extern "C" {
    pub fn rwl_loc_rdl(id: ID) -> ER;
    pub fn rwl_loc_wrl(id: ID) -> ER;
    pub fn rwl_ploc_rdl(id: ID) -> ER;
    pub fn rwl_ploc_wrl(id: ID) -> ER;
    pub fn rwl_unl_rwl(id: ID) -> ER;
    pub fn rwl_acre_rwl() -> ER_ID;
    pub fn rwl_del_rwl(id: ID) -> ER;
}

I've been trying to find information about what solid_asp3 rwl_loc_rdl does, but I struggled to find sources of its implementation. Do you know where I could go to to find solid_asp3's internal implementation of rwl_loc_rdl (or do you happen to know the max reader count for solid_asp3)?

Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't copy/paste method bodies, just link to them if you want to reference something specific. Much easier to follow the conversation if there's not large walls of code.

Should there be an assertion here that does something similar to rwlock/futex.rs with erroring with the message "too many active read locks on RwLock" when we go beyond the max reader count? I'm not sure how to test this locally or if CI would be able to run the max reader test we have here on the no_threads impl to see if it works as intended in.

Yes, I think it would be good to use checked_ arithmetic operations and panic if we overflow the counter, rather than depending on whether overflow-checks are enabled for soundness.

Should I just set MAX_READER to usize::MAX - (1 << 4) for targets associated with rwlock/queue.rs? (this will make the test run slower on 64 bit machines though, wouldn't it?)

I don't think we can afford to run up to ~2^64 iterations, that's going to be too slow. Asserting a smaller limit on those targets (e.g., 2^32 or so) should be fine though. In cfg!(miri) I'd recommend asserting we can acquire (say) 100 reader locks to keep the test fast enough.

If we set MAX_READER to usize::MAX - (1 << 4), would it be possible for this to stack overflow as all allocation for this RwLock impl is stack allocated?

I don't think so? The stack allocation for the reader locks is only if we're parked (i.e., waiting for some other thread), right?

solid_asp3

Per https://doc.rust-lang.org/nightly/rustc/platform-support/kmc-solid.html, @kawadakk is the target maintainer. Can you confirm what we should expect from the RwLock implementation on that target?

Let's assume 2^30 for now if we don't hear otherwise.

#[test]
fn test_rwlock_max_readers() {
let mut read_lock_ctr: u32 = 0;
let rwlock: RwLock<i32> = RwLock::new(0);

const MAX_READERS: u32 = cfg_select! {
miri => 100,
any(
all(target_os = "windows", not(target_vendor = "win7")),
target_os = "linux",
target_os = "android",
target_os = "freebsd",
target_os = "openbsd",
target_os = "dragonfly",
target_os = "fuchsia",
all(target_family = "wasm", target_feature = "atomics"),
target_os = "hermit",
target_os = "motor",
) => {
(1 << 30) - 2
},
any(
target_family = "unix",
all(target_os = "windows", target_vendor = "win7"),
all(target_vendor = "fortanix", target_env = "sgx"),
target_os = "xous",
target_os = "teeos",
) => {
u32::MAX
},
target_os = "solid_asp3" => {
(1 << 30)
},
_ => {
u32::MAX
}
};

while read_lock_ctr < MAX_READERS {
let lock = rwlock.read();
mem::forget(lock);
read_lock_ctr += 1;
}

assert_eq!(read_lock_ctr, MAX_READERS);
}
Loading