[libcxx-commits] [libcxx] 261d21f - [libc++] Fixes a flaky test.

[Mark de Wever via libcxx-commits]

Author: Mark de Wever
Date: 2023-02-17T18:22:09+01:00
New Revision: 261d21f8c3754e148eaf6608d359ec6e56fa276c

URL: https://github.com/llvm/llvm-project/commit/261d21f8c3754e148eaf6608d359ec6e56fa276c
DIFF: https://github.com/llvm/llvm-project/commit/261d21f8c3754e148eaf6608d359ec6e56fa276c.diff

LOG: [libc++] Fixes a flaky test.

While investigating the flaky tests on the mac backdeployment targets
it seems the tests are different than suggested in the LWG issue.
That tests "does work", grabs the mutex, marks the task as done, and
finally calls the notifier.
Our test emulated "does work" after the notification, effectively
sleeping with a lock held. This has been fixed.

A second improvement is that the test fails when, due to OS
scheduling, the condition variable is not used in the main thread.

During discussing the approach of the patch with @ldionne, he
noticed the real reason why the test is flaky; the Apple
backdeployment targets use a dylib, which does not contain the
fix. So the test can't be tested on that platform; it only
proves the LWG issue was correct and the Standard contained
a bug.

Reviewed By: ldionne, #libc

Differential Revision: https://reviews.llvm.org/D143816

Added: 
    

Modified: 
    libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp

Removed: 
    


################################################################################
diff  --git a/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
index a49a4385f6b8..9ba509d40be6 100644
--- a/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
+++ b/libcxx/test/std/thread/thread.condition/notify_all_at_thread_exit_lwg3343.pass.cpp
@@ -8,11 +8,15 @@
 //
 // UNSUPPORTED: no-threads
 
-// ALLOW_RETRIES: 3
-
 // notify_all_at_thread_exit(...) requires move semantics to transfer the unique_lock.
 // UNSUPPORTED: c++03
 
+// The fix of LWG3343 is done in the dylib. That means Apple backdeployment
+// targets remain broken. Due to the nature of the test, testing on a broken
+// system does not guarantee that the test fails, so the test can't use XFAIL.
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{.+}}
+// UNSUPPORTED: use_system_cxx_lib && target={{.+}}-apple-macosx11.{{.+}}
+
 // This is a regression test for LWG3343.
 //
 // <condition_variable>
@@ -29,6 +33,8 @@
 #include <mutex>
 #include <thread>
 
+int condition_variable_lock_skipped_counter = 0;
+
 union X {
     X() : cv_() {}
     ~X() {}
@@ -46,11 +52,13 @@ void test()
 
     for (int i = 0; i < N; ++i) {
         std::thread t = support::make_test_thread([&] {
-            // Signal thread completion
+            // Emulate work being done.
+            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+
+            // Signal thread completion.
             std::unique_lock<std::mutex> lk(m);
             --threads_active;
             std::notify_all_at_thread_exit(x.cv_, std::move(lk));
-            std::this_thread::sleep_for(std::chrono::milliseconds(1));
         });
         t.detach();
     }
@@ -62,6 +70,15 @@ void test()
     // unlocks `m`.
     {
         std::unique_lock<std::mutex> lk(m);
+        // Due to OS scheduling the workers might have terminated when this
+        // code is reached. In that case the wait will not sleep and the call
+        // to notify_all_at_thread_exit has no effect; the condition variable
+        // will not be used here.
+        //
+        // Keep track of how often that happens, if too often the test needs
+        // to be improved.
+        if(threads_active == 0)
+            ++condition_variable_lock_skipped_counter;
         x.cv_.wait(lk, [&]() { return threads_active == 0; });
     }
 
@@ -92,5 +109,9 @@ int main(int, char**)
         test();
     }
 
+    // The threshold is arbitrary, it just makes sure the notification is
+    // tested a reasonable number of times.
+    assert(condition_variable_lock_skipped_counter < 250);
+
     return 0;
 }