[libcxx-commits] [PATCH] D99175: [libcxx] [test] Make the condvar wait_for tests less brittle
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 23 06:51:50 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.
Comment at: libcxx/test/std/thread/thread.condition/thread.condition.condvar/wait_for.pass.cpp:50
+ cv.wait_for(lk, wait_end - Clock::now()) == std::cv_status::no_timeout)
Clock::time_point t1 = Clock::now();
What happens with the subtraction when `wait_end < Clock::now()`?
(This could happen because of a libc++ bug; or more likely it could happen because `wait_for` woke spuriously after 249.9ms and then it took us 0.2ms to get around to calling `Clock::now()` again.)
But also, there's no way this code should ever be intentionally waiting 250ms even once. The main thread is just waiting for line 44's `test1=1;` to happen, and then it //immediately// sets `test2=1` and notifies (line 76).
I think the only way that our line 48/49 can ever time out if if it entirely //misses// the main thread's notify due to not being asleep yet. Which actually probably happens a lot. But if the //point// of this test is to time out, then the main thread really shouldn't be notifying //at all ever// (and the timeout should probably be shorter, say 50ms). And if that's //not// the point of this test, then the main thread should be notifying only //after// `f` has gone to sleep, so the notify doesn't get missed. I think the straightforward way to do that is to swap lines 75+76 so that the main thread notifies under the lock. (That way, there's no semantic race condition where `f` spuriously wakes, then `main` notifies (without holding the lock) which races with `f`'s going back to sleep (under the lock).
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits