Page MenuHomePhabricator

[libcxx testing] Make three locking tests more reliable
ClosedPublic

Authored by davezarzycki on May 5 2020, 4:45 AM.

Details

Summary

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.

Diff Detail

Event Timeline

davezarzycki created this revision.May 5 2020, 4:45 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 5 2020, 4:45 AM
davezarzycki edited the summary of this revision. (Show Details)

Ran the patch through clang-format and then manually fixed style problems that clang-format did not fix.

ldionne accepted this revision.May 5 2020, 8:49 AM

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.

This revision is now accepted and ready to land.May 5 2020, 8:49 AM

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:

  1. 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.
  2. 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.

davezarzycki retitled this revision from [libcxx testing] Make try_lock_shared_until.pass.cpp more reliable to [libcxx testing] Make three locking tests more reliable.
davezarzycki edited the summary of this revision. (Show Details)

Fixed two more tests as an example of how to fix the other thread tests.

Ran through git clang-format to fix formatting issues that predate these fixes.

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.

ldionne accepted this revision.May 7 2020, 8:35 AM

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.

I'm entirely fine with that approach! Thanks a lot for taking a look.