The challenge with measuring time in tests is that slow and/or busy machines can cause tests to fail in unexpected ways. After this change, three tests should be much more robust. The only remaining and tiny race that I can think of is preemption after --countDown. That being said, the race isn't fixable because the standard library doesn't provide a way to count threads that are waiting to acquire a lock.
ldionne EricWF howard.hinnant mclow.lists
- Group Reviewers
- rG4f4ce13944b8: [libcxx testing] Make three locking tests more reliable
Thanks a lot for taking a look! These tests are a pain because they keep breaking CI.
Would it make sense to apply a similar transformation to other time-sensitive tests? Off the top of my head, I can find:
libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/lock.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/lock_shared.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_for.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_shared.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_shared_for.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_shared_until.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock_until.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.class/try_lock.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.class/try_lock_for.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.class/try_lock_until.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/lock.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/try_lock.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/try_lock_for.pass.cpp: libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/try_lock_until.pass.cpp:
I'm not sure which ones of these would make sense for this transformation, but would you mind taking a quick look? I think most of these tests are very similar to the one you modified.
Hi @ldionne – Ya, it looks like a lot of those tests need fixing. The "tolerance" goal is within them is fundamentally flawed. These tests are not testing "real time" APIs where one can hopefully reason about precise timing. These APIs are best-effort APIs and on a slow and/or heavily loaded machine, best-effort can get really slow. I don't have the time to fix all of the tests but here is the gist of what needs fixing is twofold:
- Use an atomic variable to ensure that all the created threads have started. This isn't perfect, but it's far better than not verifying that they've started.
- Think hard about each "tolerance" test and convert it to either >= WaitTime or <= WaitTime
I'd also like to add that there seem to be other issues. For example: lock_shared.pass.cpp has two tests, and the latter probably meant to test that a writer lock must wait for readers to finish, but instead it "tests" that a reader lock "waited" (the 'q' thread) but due to signed arithmetic, a negative wait time passes the tolerance test. :-( I'll supply a fix, but really, I don't have the time to audit all of these tests.
Hi @ldionne – I've looked a few more tests that use lit's ALLOW_RETRIES feature. I don't think this is a straightforward scenario. While the flawed assumptions are often the same, the fixes are not. Do you want 46 Phab requests? Personally speaking, this seems like one of those cases where the cost of code review discourages bug fixes. If I create decent enough commit messages, would you be open to post-commit review for these test fixes? After all, the tests are already buggy, and we can always revert back to the known buggy version that we have today.