[libc++] Fixes a flaky test.

[Mordante]

Summary

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.

[huixie90]

If all the threads exit before the cv.wait is called. cv.wait would immediately return as the predicate returns true.

[Mordante]

CI fixes.

[Mordante]

In D143816#4120396, @huixie90 wrote:

If all the threads exit before the cv.wait is called. cv.wait would immediately return as the predicate returns true.

Good point, I had looked at the overload without a predicate.

Still I think this test is better, we want to make sure notify_all_at_thread_exit does a notification. Depending on the OS scheduling the previous test may pass without any notification. Which would happen when the predicate is true at its first evaluation.

[huixie90]

In D143816#4120464, @Mordante wrote:

In D143816#4120396, @huixie90 wrote:

If all the threads exit before the cv.wait is called. cv.wait would immediately return as the predicate returns true.

Good point, I had looked at the overload without a predicate.

Still I think this test is better, we want to make sure notify_all_at_thread_exit does a notification. Depending on the OS scheduling the previous test may pass without any notification. Which would happen when the predicate is true at its first evaluation.

The fact that the test can hang might indicate there might be a problem either in our libc++ , or the OS, if we cannot prove that the test is not correct.

Maybe we should try to figure that out as well

[Mordante]

In D143816#4120523, @huixie90 wrote:

In D143816#4120464, @Mordante wrote:

In D143816#4120396, @huixie90 wrote:

If all the threads exit before the cv.wait is called. cv.wait would immediately return as the predicate returns true.

Good point, I had looked at the overload without a predicate.

Still I think this test is better, we want to make sure notify_all_at_thread_exit does a notification. Depending on the OS scheduling the previous test may pass without any notification. Which would happen when the predicate is true at its first evaluation.

The fact that the test can hang might indicate there might be a problem either in our libc++ , or the OS, if we cannot prove that the test is not correct.

Maybe we should try to figure that out as well

I fully agree. That's why the review is still in draft state. But I wanted to test whether this fix passes the CI for all platforms. I'm still looking into it.

[ldionne]

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

12

I think this needs to be marked as UNSUPPORTED on the back-deployment target. The fix is in the dylib so we should expect it to fail on back-deployment targets.

I can't believe I didn't think of that earlier, I'm sorry I hadn't realized the original fix was in thread.cpp, not in <thread>.

[Mordante]

Rewritten the test based on a private discussion with @ldionne.

[Mordante]

Minor comment improvement.

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

18

@ldionne please verify whether these targets are correct. These are the same values I use for the format backdeployment tests.

[huixie90]

hmm. I am not sure if I understand why we need to change the test. According to the description, it is not the test that is flaky. Instead, it is the system under test (the dylib which contains the bug) that is flaky.
So the test does exact what it should do: detect the bug. The fact that it fails very frequently probably means that it does a good job.

the added assertion might actually makes the test flaky since that totally depends on the scheduler.

[Mordante]

In D143816#4131395, @huixie90 wrote:

hmm. I am not sure if I understand why we need to change the test. According to the description, it is not the test that is flaky. Instead, it is the system under test (the dylib which contains the bug) that is flaky.
So the test does exact what it should do: detect the bug. The fact that it fails very frequently probably means that it does a good job.

the added assertion might actually makes the test flaky since that totally depends on the scheduler.

There are two reasons for the change:

1. The typical use case of notify_all_at_thread_exit is to issue it at the end of the thread. In the original case the worker thread was sleeping while holding a mutex. Something which is not a typical use case.
2. The test might never actually test that notify_all_at_thread_exit is called. Testing on Mac revealed it did happen, but was extremely rare. So if this fails 25% of the time on other platforms we should look at how to make the test more realistic, longer sleep, yielding at the start of the thread execution, add more thread, etc. So this is basically a test that the test tests what it expects.

I agree these improvements are not related to the "fix", so maybe it would be better to commit them separately.

[ldionne]

I think I like those changes, however I am wondering -- does the test still fail as reliably with those changes? Did you run it on the backdeployment host with those changes and you could see it fail pretty much as often as before? If so, this LGTM. If this makes the test not reproduce (or reproduce much less frequently), then we're reducing its effectiveness and I'd argue we shouldn't.

LGTM assuming this does not power-down the test.

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

18

Yeah, that sounds fine to me. I think 12.0 would fail too, but this can be fixed in bulk when we add testing for 12.0 in backdeployment. Other tests will also need fixing anyway.

78–79

[Mordante]

In D143816#4132993, @ldionne wrote:

I think I like those changes, however I am wondering -- does the test still fail as reliably with those changes? Did you run it on the backdeployment host with those changes and you could see it fail pretty much as often as before? If so, this LGTM. If this makes the test not reproduce (or reproduce much less frequently), then we're reducing its effectiveness and I'd argue we shouldn't.

LGTM assuming this does not power-down the test.

I didn't run this version on back-deployment, I can do that tomorrow. During testing I've tested with a version where I removed the sleep entirely and I got the feeling it made the test less stable on Mac. At that time I didn't investigate it further and I wasn't aware of the real issue on the Mac.

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

78–79

Thanks, when is indeed not intended.

[Mordante]

In D143816#4132993, @ldionne wrote:

I think I like those changes, however I am wondering -- does the test still fail as reliably with those changes? Did you run it on the backdeployment host with those changes and you could see it fail pretty much as often as before? If so, this LGTM. If this makes the test not reproduce (or reproduce much less frequently), then we're reducing its effectiveness and I'd argue we shouldn't.

Well it seems the CI hates me ;-) The original test, the sleep before mutex, and even removing the sleep are running stable on the Mac. I will try again another time.

LGTM assuming this does not power-down the test.

It shouldn't.

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

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // 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>
 //
 // void notify_all_at_thread_exit(condition_variable& cond, unique_lock<mutex> lk);
 
 #include "make_test_thread.h"
 #include "test_macros.h"
 
 #include <condition_variable>
 #include <cassert>
 #include <chrono>
 #include <memory>
 #include <mutex>
 #include <thread>
 
+int condition_variable_lock_skipped_counter = 0;
+
 union X {
     X() : cv_() {}
     ~X() {}
     std::condition_variable cv_;
     unsigned char bytes_[sizeof(std::condition_variable)];
 };
 
 void test()
 {
     constexpr int N = 3;
 
     X x;
     std::mutex m;
     int threads_active = N;
 
     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();
     }
 
     // Wait until all threads complete, i.e. until they've all
     // decremented `threads_active` and then unlocked `m` at thread exit.
     // It is possible that this `wait` may spuriously wake up,
     // but it won't be able to continue until the last thread
     // 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; });
     }
 
     // Destroy the condition_variable and shred the bytes.
     // Simulate reusing the memory for something else.
     x.cv_.~condition_variable();
     for (unsigned char& c : x.bytes_) {
         c = 0xcd;

 }
 
 int main(int, char**)
 {
     for (int i = 0; i < 1000; ++i) {
         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;
 }