[libc++] Refactor flaky tests for std::shared_lock

[ldionne]

This makes the tests non-flaky.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

This makes the tests non-flaky.

---

Full diff: https://github.com/llvm/llvm-project/pull/91779.diff

3 Files Affected:

• (modified) libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.cons/mutex.pass.cpp (+65-67)
• (modified) libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/lock.pass.cpp (+82-57)
• (modified) libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/try_lock.pass.cpp (+93-41)

diff --git a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.cons/mutex.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.cons/mutex.pass.cpp
index 4940041bcf965..ece330134f2cd 100644
--- a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.cons/mutex.pass.cpp
+++ b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.cons/mutex.pass.cpp
@@ -5,10 +5,9 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-//
+
 // UNSUPPORTED: no-threads
 // UNSUPPORTED: c++03, c++11
-// ALLOW_RETRIES: 2
 
 // <shared_mutex>
 
@@ -19,9 +18,8 @@
 // template<class _Mutex> shared_lock(shared_lock<_Mutex>)
 //     -> shared_lock<_Mutex>;  // C++17
 
+#include <atomic>
 #include <cassert>
-#include <chrono>
-#include <cstdlib>
 #include <shared_mutex>
 #include <thread>
 #include <vector>
@@ -29,77 +27,77 @@
 #include "make_test_thread.h"
 #include "test_macros.h"
 
-typedef std::chrono::system_clock Clock;
-typedef Clock::time_point time_point;
-typedef Clock::duration duration;
-typedef std::chrono::milliseconds ms;
-typedef std::chrono::nanoseconds ns;
-
-ms WaitTime = ms(250);
-
-// Thread sanitizer causes more overhead and will sometimes cause this test
-// to fail. To prevent this we give Thread sanitizer more time to complete the
-// test.
-#if !defined(TEST_IS_EXECUTED_IN_A_SLOW_ENVIRONMENT)
-ms Tolerance = ms(50);
-#else
-ms Tolerance = ms(50 * 5);
-#endif
+struct Monitor {
+  bool lock_shared_called   = false;
+  bool unlock_shared_called = false;
+};
 
-std::shared_timed_mutex m;
+struct TrackedMutex {
+  Monitor* monitor = nullptr;
 
-void f()
-{
-    time_point t0 = Clock::now();
-    time_point t1;
-    {
-    std::shared_lock<std::shared_timed_mutex> ul(m);
-    t1 = Clock::now();
-    }
-    ns d = t1 - t0 - WaitTime;
-    assert(d < Tolerance);  // within tolerance
-}
+  void lock_shared() {
+    if (monitor != nullptr)
+      monitor->lock_shared_called = true;
+  }
+  void unlock_shared() {
+    if (monitor != nullptr)
+      monitor->unlock_shared_called = true;
+  }
+};
 
-void g()
-{
-    time_point t0 = Clock::now();
-    time_point t1;
-    {
-    std::shared_lock<std::shared_timed_mutex> ul(m);
-    t1 = Clock::now();
-    }
-    ns d = t1 - t0;
-    assert(d < Tolerance);  // within tolerance
-}
+template <class Mutex>
+void test() {
+  // Basic sanity test
+  {
+    Mutex mutex;
+    std::vector<std::thread> threads;
+    std::atomic<bool> ready(false);
+    for (int i = 0; i != 5; ++i) {
+      threads.push_back(support::make_test_thread([&] {
+        while (!ready) {
+          // spin
+        }
 
-int main(int, char**)
-{
-    std::vector<std::thread> v;
-    {
-        m.lock();
-        for (int i = 0; i < 5; ++i)
-            v.push_back(support::make_test_thread(f));
-        std::this_thread::sleep_for(WaitTime);
-        m.unlock();
-        for (auto& t : v)
-            t.join();
-    }
-    {
-        m.lock_shared();
-        for (auto& t : v)
-            t = support::make_test_thread(g);
-        std::thread q = support::make_test_thread(f);
-        std::this_thread::sleep_for(WaitTime);
-        m.unlock_shared();
-        for (auto& t : v)
-            t.join();
-        q.join();
+        std::shared_lock<Mutex> lock(mutex);
+        assert(lock.owns_lock());
+      }));
     }
 
+    ready = true;
+    for (auto& t : threads)
+      t.join();
+  }
+
+  // Test CTAD
+  {
+#if TEST_STD_VER >= 17
+    Mutex mutex;
+    std::shared_lock lock(mutex);
+    static_assert(std::is_same<decltype(lock), std::shared_lock<Mutex>>::value);
+#endif
+  }
+}
+
+int main(int, char**) {
 #if TEST_STD_VER >= 17
-    std::shared_lock sl(m);
-    static_assert((std::is_same<decltype(sl), std::shared_lock<decltype(m)>>::value), "" );
+  test<std::shared_mutex>();
 #endif
+  test<std::shared_timed_mutex>();
+  test<TrackedMutex>();
+
+  // Use shared_lock with a dummy mutex class that tracks whether each
+  // operation has been called or not.
+  {
+    Monitor monitor;
+    TrackedMutex mutex{&monitor};
+
+    std::shared_lock<TrackedMutex> lock(mutex);
+    assert(monitor.lock_shared_called);
+    assert(lock.owns_lock());
+
+    lock.unlock();
+    assert(monitor.unlock_shared_called);
+  }
 
   return 0;
 }
diff --git a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/lock.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/lock.pass.cpp
index edb7c42356ace..0dd385e56f52f 100644
--- a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/lock.pass.cpp
+++ b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/lock.pass.cpp
@@ -5,10 +5,9 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-//
+
 // UNSUPPORTED: no-threads
 // UNSUPPORTED: c++03, c++11
-// ALLOW_RETRIES: 2
 
 // <shared_mutex>
 
@@ -16,10 +15,8 @@
 
 // void lock();
 
+#include <atomic>
 #include <cassert>
-#include <chrono>
-#include <cstdlib>
-#include <mutex>
 #include <shared_mutex>
 #include <system_error>
 #include <thread>
@@ -28,71 +25,99 @@
 #include "make_test_thread.h"
 #include "test_macros.h"
 
-std::shared_timed_mutex m;
+struct Monitor {
+  bool lock_shared_called   = false;
+  bool unlock_shared_called = false;
+};
 
-typedef std::chrono::system_clock Clock;
-typedef Clock::time_point time_point;
-typedef Clock::duration duration;
-typedef std::chrono::milliseconds ms;
-typedef std::chrono::nanoseconds ns;
+struct TrackedMutex {
+  Monitor* monitor = nullptr;
 
-ms WaitTime = ms(250);
+  void lock_shared() {
+    if (monitor != nullptr)
+      monitor->lock_shared_called = true;
+  }
+  void unlock_shared() {
+    if (monitor != nullptr)
+      monitor->unlock_shared_called = true;
+  }
+};
 
-// Thread sanitizer causes more overhead and will sometimes cause this test
-// to fail. To prevent this we give Thread sanitizer more time to complete the
-// test.
-#if !defined(TEST_IS_EXECUTED_IN_A_SLOW_ENVIRONMENT)
-ms Tolerance = ms(25);
-#else
-ms Tolerance = ms(25 * 5);
-#endif
+template <class Mutex>
+void test() {
+  // Basic sanity test
+  {
+    Mutex mutex;
+    std::vector<std::thread> threads;
+    std::atomic<bool> ready(false);
+    for (int i = 0; i != 5; ++i) {
+      threads.push_back(support::make_test_thread([&] {
+        while (!ready) {
+          // spin
+        }
 
+        std::shared_lock<Mutex> lock(mutex, std::defer_lock);
+        lock.lock();
+        assert(lock.owns_lock());
+      }));
+    }
+
+    ready = true;
+    for (auto& t : threads)
+      t.join();
+  }
 
-void f()
-{
-    std::shared_lock<std::shared_timed_mutex> lk(m, std::defer_lock);
-    time_point t0 = Clock::now();
-    lk.lock();
-    time_point t1 = Clock::now();
-    assert(lk.owns_lock() == true);
-    ns d = t1 - t0 - WaitTime;
-    assert(d < Tolerance);  // within tolerance
+  // Try locking the same shared_lock again in the same thread. This should throw an exception.
+  {
+    Mutex mutex;
+    std::shared_lock<Mutex> lock(mutex, std::defer_lock);
+    lock.lock();
+    assert(lock.owns_lock());
 #ifndef TEST_HAS_NO_EXCEPTIONS
-    try
-    {
-        lk.lock();
-        assert(false);
-    }
-    catch (std::system_error& e)
-    {
-        assert(e.code().value() == EDEADLK);
+    try {
+      lock.lock();
+      assert(false);
+    } catch (std::system_error const& e) {
+      assert(e.code() == std::errc::resource_deadlock_would_occur);
     }
 #endif
-    lk.unlock();
-    lk.release();
+  }
+
+  // Try locking a shared_lock that isn't associated to any mutex. This should throw an exception.
+  {
+    std::shared_lock<Mutex> lock; // no associated mutex
 #ifndef TEST_HAS_NO_EXCEPTIONS
-    try
-    {
-        lk.lock();
-        assert(false);
-    }
-    catch (std::system_error& e)
-    {
-        assert(e.code().value() == EPERM);
+    try {
+      lock.lock();
+      assert(false);
+    } catch (std::system_error const& e) {
+      assert(e.code() == std::errc::operation_not_permitted);
     }
 #endif
+  }
 }
 
-int main(int, char**)
-{
-    m.lock();
-    std::vector<std::thread> v;
-    for (int i = 0; i < 5; ++i)
-        v.push_back(support::make_test_thread(f));
-    std::this_thread::sleep_for(WaitTime);
-    m.unlock();
-    for (auto& t : v)
-        t.join();
+int main(int, char**) {
+#if TEST_STD_VER >= 17
+  test<std::shared_mutex>();
+#endif
+  test<std::shared_timed_mutex>();
+  test<TrackedMutex>();
+
+  // Use shared_lock with a dummy mutex class that tracks whether each
+  // operation has been called or not.
+  {
+    Monitor monitor;
+    TrackedMutex mutex{&monitor};
+
+    std::shared_lock<TrackedMutex> lock(mutex, std::defer_lock);
+    lock.lock();
+    assert(monitor.lock_shared_called);
+    assert(lock.owns_lock());
+
+    lock.unlock();
+    assert(monitor.unlock_shared_called);
+  }
 
   return 0;
 }
diff --git a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/try_lock.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/try_lock.pass.cpp
index 0e707fcf2d50a..daa5ee8b56b8b 100644
--- a/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/try_lock.pass.cpp
+++ b/libcxx/test/std/thread/thread.mutex/thread.lock/thread.lock.shared/thread.lock.shared.locking/try_lock.pass.cpp
@@ -5,11 +5,9 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
-//
+
 // UNSUPPORTED: no-threads
 // UNSUPPORTED: c++03, c++11
-//
-// ALLOW_RETRIES: 2
 
 // <shared_mutex>
 
@@ -17,60 +15,114 @@
 
 // bool try_lock();
 
+#include <atomic>
 #include <cassert>
-#include <mutex>
 #include <shared_mutex>
 #include <system_error>
+#include <thread>
+#include <vector>
 
+#include "make_test_thread.h"
 #include "test_macros.h"
 
-bool try_lock_called = false;
+struct Monitor {
+  bool try_lock_shared_called = false;
+  bool unlock_shared_called   = false;
+};
 
-struct mutex
-{
-    bool try_lock_shared()
-    {
-        try_lock_called = !try_lock_called;
-        return try_lock_called;
-    }
-    void unlock_shared() {}
+struct TrackedMutex {
+  Monitor* monitor = nullptr;
+
+  bool try_lock_shared() {
+    if (monitor != nullptr)
+      monitor->try_lock_shared_called = true;
+    return true;
+  }
+  void unlock_shared() {
+    if (monitor != nullptr)
+      monitor->unlock_shared_called = true;
+  }
 };
 
-mutex m;
+template <class Mutex>
+void test() {
+  // Basic sanity test
+  {
+    Mutex mutex;
+    std::vector<std::thread> threads;
+    std::atomic<bool> ready(false);
+    for (int i = 0; i != 5; ++i) {
+      threads.push_back(support::make_test_thread([&] {
+        while (!ready) {
+          // spin
+        }
 
-int main(int, char**)
-{
-    std::shared_lock<mutex> lk(m, std::defer_lock);
-    assert(lk.try_lock() == true);
-    assert(try_lock_called == true);
-    assert(lk.owns_lock() == true);
-#ifndef TEST_HAS_NO_EXCEPTIONS
-    try
-    {
-        TEST_IGNORE_NODISCARD lk.try_lock();
-        assert(false);
+        std::shared_lock<Mutex> lock(mutex, std::defer_lock);
+        bool result = lock.try_lock();
+        assert(result);
+        assert(lock.owns_lock());
+      }));
     }
-    catch (std::system_error& e)
-    {
-        assert(e.code().value() == EDEADLK);
+
+    ready = true;
+    for (auto& t : threads)
+      t.join();
+  }
+
+  // Make sure that we throw an exception if we try to re-lock a mutex that is
+  // already locked by the current thread.
+  {
+    Mutex mutex;
+
+    std::shared_lock<Mutex> lock(mutex, std::defer_lock);
+    assert(lock.try_lock());
+    assert(lock.owns_lock());
+#ifndef TEST_HAS_NO_EXCEPTIONS
+    try {
+      TEST_IGNORE_NODISCARD lock.try_lock();
+      assert(false);
+    } catch (std::system_error const& e) {
+      assert(e.code() == std::errc::resource_deadlock_would_occur);
     }
 #endif
-    lk.unlock();
-    assert(lk.try_lock() == false);
-    assert(try_lock_called == false);
-    assert(lk.owns_lock() == false);
-    lk.release();
+  }
+
+  // Make sure that we throw an exception if we try to lock a shared_lock
+  // that is not associated to any mutex.
+  {
+    std::shared_lock<Mutex> lock; // not associated to a mutex
 #ifndef TEST_HAS_NO_EXCEPTIONS
-    try
-    {
-        TEST_IGNORE_NODISCARD lk.try_lock();
-        assert(false);
-    }
-    catch (std::system_error& e)
-    {
-        assert(e.code().value() == EPERM);
+    try {
+      TEST_IGNORE_NODISCARD lock.try_lock();
+      assert(false);
+    } catch (std::system_error const& e) {
+      assert(e.code() == std::errc::operation_not_permitted);
     }
 #endif
+  }
+}
+
+int main(int, char**) {
+#if TEST_STD_VER >= 17
+  test<std::shared_mutex>();
+#endif
+  test<std::shared_timed_mutex>();
+  test<TrackedMutex>();
+
+  // Use shared_lock with a dummy mutex class that tracks whether each
+  // operation has been called or not.
+  {
+    Monitor monitor;
+    TrackedMutex mutex{&monitor};
+
+    std::shared_lock<TrackedMutex> lock(mutex, std::defer_lock);
+    bool result = lock.try_lock();
+    assert(result);
+    assert(monitor.try_lock_shared_called);
+    assert(lock.owns_lock());
 
+    lock.unlock();
+    assert(monitor.unlock_shared_called);
+  }
   return 0;
 }

[huixie90]

LGTM!

[ldionne]

Merging since CI failures seem to be the usual spurious "interrupted by user" failure.