Skip to content

Commit

Permalink
fix memory ordering bug + bad test
Browse files Browse the repository at this point in the history
This commit fixes a memory ordering bug in the futex implementation
(`Relaxed` -> `Release` on `downgrade`).

This commit also removes a badly written test that deadlocked and
replaces it with a more reasonable test based on an already-tested
`downgrade` test from the parking-lot crate.
  • Loading branch information
connortsui20 committed Nov 16, 2024
1 parent 3d191b5 commit 84fd95c
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 48 deletions.
79 changes: 32 additions & 47 deletions library/std/src/sync/rwlock/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use rand::Rng;
use crate::sync::atomic::{AtomicUsize, Ordering};
use crate::sync::mpsc::channel;
use crate::sync::{
Arc, Barrier, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard,
RwLockWriteGuard, TryLockError,
Arc, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard, RwLockWriteGuard,
TryLockError,
};
use crate::thread;

Expand Down Expand Up @@ -511,57 +511,42 @@ fn test_downgrade_basic() {
}

#[test]
fn test_downgrade_readers() {
// This test creates 1 writing thread and `R` reader threads doing `N` iterations.
const R: usize = 10;
const N: usize = if cfg!(target_pointer_width = "64") { 100 } else { 20 };
fn test_downgrade_observe() {
// Taken from the test `test_rwlock_downgrade` from:
// https://github.com/Amanieu/parking_lot/blob/master/src/rwlock.rs

// The writer thread will constantly update the value inside the `RwLock`, and this test will
// only pass if every reader observes all values between 0 and `N`.
let rwlock = Arc::new(RwLock::new(0));
let barrier = Arc::new(Barrier::new(R + 1));

// Create the writing thread.
let r_writer = rwlock.clone();
let b_writer = barrier.clone();
thread::spawn(move || {
for i in 0..N {
let mut write_guard = r_writer.write().unwrap();
*write_guard = i;
const W: usize = if cfg!(target_pointer_width = "64") { 100 } else { 20 };
const N: usize = 100;

let read_guard = RwLockWriteGuard::downgrade(write_guard);
assert_eq!(*read_guard, i);
// This test spawns `W` writer threads, where each will increment a counter `N` times, ensuring
// that the value they wrote has not changed after downgrading.

// Wait for all readers to observe the new value.
b_writer.wait();
}
});
let rw = Arc::new(RwLock::new(0));

for _ in 0..R {
let rwlock = rwlock.clone();
let barrier = barrier.clone();
thread::spawn(move || {
// Every reader thread needs to observe every value up to `N`.
for i in 0..N {
let read_guard = rwlock.read().unwrap();
assert_eq!(*read_guard, i);
drop(read_guard);

// Wait for everyone to read and for the writer to change the value again.
barrier.wait();

// Spin until the writer has changed the value.
loop {
let read_guard = rwlock.read().unwrap();
assert!(*read_guard >= i);

if *read_guard > i {
break;
}
// Spawn the writers that will do `W * N` operations and checks.
let handles: Vec<_> = (0..W)
.map(|_| {
let rw = rw.clone();
thread::spawn(move || {
for _ in 0..N {
// Increment the counter.
let mut write_guard = rw.write().unwrap();
*write_guard += 1;
let cur_val = *write_guard;

// Downgrade the lock to read mode, where the value protected cannot be modified.
let read_guard = RwLockWriteGuard::downgrade(write_guard);
assert_eq!(cur_val, *read_guard);
}
}
});
})
})
.collect();

for handle in handles {
handle.join().unwrap();
}

assert_eq!(*rw.read().unwrap(), W * N);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/sys/sync/rwlock/futex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl RwLock {
#[inline]
pub unsafe fn downgrade(&self) {
// Removes all write bits and adds a single read bit.
let state = self.state.fetch_add(DOWNGRADE, Relaxed);
let state = self.state.fetch_add(DOWNGRADE, Release);
debug_assert!(is_write_locked(state), "RwLock must be write locked to call `downgrade`");

if has_readers_waiting(state) {
Expand Down

0 comments on commit 84fd95c

Please sign in to comment.