Fix buggy test_pthread() condvar test

There's no guarantee that a condition variable is stateful. The docs for
`Condvar::notify_one()` actually say the opposite:

> If there is a blocked thread on this condition variable, then it will be woken
> up from its call to wait or wait_timeout. Calls to notify_one are not buffered
> in any way.

This test was relying on the main loop obtaining the lock and entering the
condition variable sleep before the thread was scheduled and got around to
notifying the condition variable. If this non-deterministic behavior was not
upheld, the test would time out since it would obtain the lock (either before or
after the variable were updated) then call `condvar.wait()` *after* the variable
had been updated and the condvar signalled, but without (atomically or even at
all) checking to see if the desired wake precondition was fulfilled. As the
child thread had already run and the wake notification was NOT buffered, there
was nothing to wake the running thread.

There really wasn't any way to salvage the test as originally written, since the
write to `ctx.val` was not in any way linked to the acquire/release of the mutex
so regardless of whether or not the main thread obtained the mutex and checked
the value precondition before calling `condvar.wait()`, the child thread's write
could have happened after the check but before the wait() call. As such, the
test has been rewritten to use `wait_while()` but then also updated to bail in
case of a timeout instead of hanging indefinitely (since neither the `ctest`
runner nor the `cargo test` harness was timing out; `cargo test` would only
report that the test had exceeded 60 seconds but as long as it was not executed
with `cargo test -- -Z --ensure-time` (which is only available under nightly),
the test would not halt.

If this test were *intentionally* written to test the scenario that was timing
out, it should be written deterministically in such a way that the main loop
did not run until after it was guaranteed that the variable had been updated
(i.e. by looping until val became 5 or waiting for an AtomicBool indicating the
update had completed to be set), but I'm not sure what the benefit in that would
be since the docs actually guarantee the opposite behavior (the notified state
is explicitly not cached/buffered).

If we have fish code written with the assumption that condvar notifications
prior to *any* call to `Condvar::wait()` *are* buffered, then that code should
of course be revisited in light of this.
This commit is contained in:
Mahmoud Al-Qudsi 2024-05-29 13:13:13 -05:00
parent 390b40e02b
commit 7bf3b57e47

View file

@ -1,6 +1,7 @@
use crate::threads::spawn; use crate::threads::spawn;
use std::sync::atomic::{AtomicI32, Ordering}; use std::sync::atomic::{AtomicI32, Ordering};
use std::sync::{Arc, Condvar, Mutex}; use std::sync::{Arc, Condvar, Mutex};
use std::time::Duration;
#[test] #[test]
fn test_pthread() { fn test_pthread() {
@ -17,15 +18,26 @@ fn test_pthread() {
let made = spawn(move || { let made = spawn(move || {
ctx2.val.fetch_add(2, Ordering::Release); ctx2.val.fetch_add(2, Ordering::Release);
ctx2.condvar.notify_one(); ctx2.condvar.notify_one();
println!("condvar signalled");
}); });
assert!(made); assert!(made);
let mut lock = mutex.lock().unwrap();
loop { let lock = mutex.lock().unwrap();
lock = ctx.condvar.wait(lock).unwrap(); let (_lock, timeout) = ctx
let v = ctx.val.load(Ordering::Acquire); .condvar
if v == 5 { .wait_timeout_while(lock, Duration::from_secs(5), |()| {
return; println!("looping with lock held");
if ctx.val.load(Ordering::Acquire) != 5 {
println!("test_pthread: value did not yet reach goal");
return true;
} }
println!("test_pthread: value did not yet reach goal") false
})
.unwrap();
if timeout.timed_out() {
panic!(concat!(
"Timeout waiting for condition variable to be notified! ",
"Does the platform support signalling a condvar without the mutex held?"
));
} }
} }