[libc++] Fix wait_on_destruct.pass.cpp hanging sometimes

[EricWF]

This test was deadlocking on my machine.

It seems to me the intention of in_async.wait(...) was to wait for the value to be set to true, which requires a call of wait(false) (waits if value matches argument).

As a drive by change scoped_lock to unique_lock, since there shouldn't be any functional difference between the two in this test.

I've addressed the issues with the in_async by switching to a condition variable instead, since my first attempt at fixing this with in_async wasn't sufficient.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Eric (EricWF)

Changes

It seems to me the intention of in_async.wait(...) was to wait for the value to be set to true, which requires a call of wait(false) (waits if value matches argument).

As a drive by change scoped_lock to unique_lock, since there shouldn't be any functional difference between the two in this test.

Alternatively, the test could be written with a condition variable directly.

---

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

1 Files Affected:

• (modified) libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp (+2-2)

diff --git a/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
index 8260ec3dfcaf4..6ab9f1d0b1afb 100644
--- a/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
+++ b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
@@ -30,11 +30,11 @@ int main(int, char**) {
   auto v                     = std::async(std::launch::async, [&in_async, value = 1]() mutable {
     in_async = true;
     in_async.notify_all();
-    std::scoped_lock thread_lock(mux);
+    std::unique_lock thread_lock(mux);
     value = 4;
     (void)value;
   });
-  in_async.wait(true);
+  in_async.wait(false);
   lock.unlock();
 
   return 0;

[EricWF]

On second thought, this is actually inadequate . Since the in_async variable can go out of scope between being set to true and having notify_all set on it.

So the condition variable approach is probably best.

[philnik777]

This test was deadlocking on my machine.

It seems to me the intention of in_async.wait(...) was to wait for the value to be set to true, which requires a call of wait(false) (waits if value matches argument).

Yes, I've got that backwards.

As a drive by change scoped_lock to unique_lock, since there shouldn't be any functional difference between the two in this test.

I've addressed the issues with the in_async by switching to a condition variable instead, since my first attempt at fixing this with in_async wasn't sufficient.

Could you elaborate? I just testted with changing in_async.wait(true) to in_async.wait(false) and it ran successfully 1000 times.

[EricWF]

This test was deadlocking on my machine.
It seems to me the intention of in_async.wait(...) was to wait for the value to be set to true, which requires a call of wait(false) (waits if value matches argument).

Yes, I've got that backwards.

As a drive by change scoped_lock to unique_lock, since there shouldn't be any functional difference between the two in this test.
I've addressed the issues with the in_async by switching to a condition variable instead, since my first attempt at fixing this with in_async wasn't sufficient.

Could you elaborate? I just testted with changing in_async.wait(true) to in_async.wait(false) and it ran successfully 1000 times.

Yeah. So there's a race on in_async. If the async thread sets the value to true, then immediately sleeps, the main thread can run until completion, when the async thread wakes up it tries to call notify_all the stack containing in_async is gone.

[philnik777]

This test was deadlocking on my machine.
It seems to me the intention of in_async.wait(...) was to wait for the value to be set to true, which requires a call of wait(false) (waits if value matches argument).

Yes, I've got that backwards.

As a drive by change scoped_lock to unique_lock, since there shouldn't be any functional difference between the two in this test.
I've addressed the issues with the in_async by switching to a condition variable instead, since my first attempt at fixing this with in_async wasn't sufficient.

Could you elaborate? I just testted with changing in_async.wait(true) to in_async.wait(false) and it ran successfully 1000 times.

Yeah. So there's a race on in_async. If the async thread sets the value to true, then immediately sleeps, the main thread can run until completion, when the async thread wakes up it tries to call notify_all the stack containing in_async is gone.

Ah, good catch!

LGTM with sleep_for removed.

[EricWF]

Thanks for the review.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp

View the diff from clang-format here.

diff --git a/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
index d2964e022..7384a195b 100644
--- a/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
+++ b/libcxx/test/std/thread/futures/futures.async/wait_on_destruct.pass.cpp
@@ -23,7 +23,6 @@
 #include <condition_variable>
 #include <thread>
 
-
 std::mutex mux;
 
 int main(int, char**) {

[EricWF]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
View the diff from clang-format here.

Woops. I've addressed the formatting mistake in a follow up commit.