[libcxx-commits] [libcxx] [libc++] std::condition_variable_any overloads accepting std::stop_token don't register for stop callbacks (PR #77099)
via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Jan 5 06:34:22 PST 2024
https://github.com/sn-rbroker created https://github.com/llvm/llvm-project/pull/77099
[The paper](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0660r10.pdf) for P0660R10 defines 3 "interruptible waits" in section 32.6.4.2 which accept a stop token. These are defined to register to be notified of a stop request on their corresponding stop token, but libc++ doesn't appear to implement this behaviour. For example, the definition of `std::condition_variable_any::wait_until` accepting a stop_token states:
```
Effects: Registers for the duration of this call *this to get notified on a stop request on stoken during this call and then equivalent to:
while (!stoken.stop_requested()) {
if (pred())
return true;
wait(lock);
}
return pred();
```
Because the "interruptible wait" calls do not register to be notified, an application which is expecting a call to "std::stop_token::request_stop()" to unblock a condition variable wait may hang unexpectedly. In other implementations this works as described in the paper, and it's not necessary to periodically wake the condition variable to re-evaluate whether the a stop has been requested, or the predicate satisfied.
This can be resolved by registering for a `std::stop_callback` on the supplied token, which then calls `notify_all()` when a stop is requested. This PR incorporates that change and updates the corresponding test cases to remove an existing workaround which was periodically notifying the condition variable.
Some additional changes are required to bring the interruptible wait calls in line with the other waits, as `__stoken.stop_requested()` needs to be evaluated while holding the inner lock, not the lock the user has passed in, to avoid hangs which can otherwise occur when the unit tests aren't periodically notifying the condition variable.
>From 38b951804699e2d2271e3cf481da190c86fe358f Mon Sep 17 00:00:00 2001
From: Richard Broker <Richard.Broker at sony.com>
Date: Fri, 15 Dec 2023 15:03:09 +0000
Subject: [PATCH] [libc++] Update std::condition_variable_any to register for
std::stop_callback for overloads accepting a std::stop_token
---
libcxx/include/condition_variable | 30 ++++++++++++++-----
.../wait_for_token_pred.pass.cpp | 7 -----
.../wait_token_pred.pass.cpp | 7 -----
.../wait_until_token_pred.pass.cpp | 7 -----
4 files changed, 23 insertions(+), 28 deletions(-)
diff --git a/libcxx/include/condition_variable b/libcxx/include/condition_variable
index cf7a570b6cb635..5ad262fb99c8ed 100644
--- a/libcxx/include/condition_variable
+++ b/libcxx/include/condition_variable
@@ -131,6 +131,7 @@ public:
#include <__mutex/mutex.h>
#include <__mutex/tag_types.h>
#include <__mutex/unique_lock.h>
+#include <__stop_token/stop_callback.h>
#include <__stop_token/stop_token.h>
#include <__utility/move.h>
#include <version>
@@ -257,23 +258,38 @@ condition_variable_any::wait_for(_Lock& __lock, const chrono::duration<_Rep, _Pe
template <class _Lock, class _Predicate>
bool condition_variable_any::wait(_Lock& __lock, stop_token __stoken, _Predicate __pred) {
- while (!__stoken.stop_requested()) {
+ shared_ptr<mutex> __mut = __mut_;
+ stop_callback __cb { __stoken, [this]{ notify_all(); } };
+ while (true) {
if (__pred())
return true;
- wait(__lock);
- }
- return __pred();
+ unique_lock<mutex> __lk { *__mut };
+ if (__stoken.stop_requested())
+ return __pred();
+ __lock.unlock();
+ unique_ptr<_Lock, __lock_external> __lxx(&__lock);
+ lock_guard<unique_lock<mutex>> __lx(__lk, adopt_lock_t());
+ __cv_.wait(__lk);
+ } // __mut_.unlock(), __lock.lock()
}
template <class _Lock, class _Clock, class _Duration, class _Predicate>
bool condition_variable_any::wait_until(
_Lock& __lock, stop_token __stoken, const chrono::time_point<_Clock, _Duration>& __abs_time, _Predicate __pred) {
- while (!__stoken.stop_requested()) {
+ shared_ptr<mutex> __mut = __mut_;
+ stop_callback __cb { __stoken, [this]{ notify_all(); } };
+ while (true) {
if (__pred())
return true;
- if (wait_until(__lock, __abs_time) == cv_status::timeout)
+ unique_lock<mutex> __lk { *__mut };
+ if (__stoken.stop_requested())
return __pred();
- }
+ __lock.unlock();
+ unique_ptr<_Lock, __lock_external> __lxx(&__lock);
+ lock_guard<unique_lock<mutex>> __lx(__lk, adopt_lock_t());
+ if (__cv_.wait_until(__lk, __abs_time) == cv_status::timeout)
+ break;
+ } // __mut_.unlock(), __lock.lock()
return __pred();
}
diff --git a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp
index fb3f0287726eea..d4aa725e3b6f00 100644
--- a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp
+++ b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp
@@ -132,15 +132,9 @@ void test() {
Lock lock{mutex};
std::atomic_bool start = false;
- std::atomic_bool done = false;
auto thread = support::make_test_thread([&]() {
start.wait(false);
ss.request_stop();
-
- while (!done) {
- cv.notify_all();
- std::this_thread::sleep_for(2ms);
- }
});
std::same_as<bool> auto r = cv.wait_for(lock, ss.get_token(), 1h, [&]() {
@@ -149,7 +143,6 @@ void test() {
return false;
});
assert(!r);
- done = true;
thread.join();
assert(lock.owns_lock());
diff --git a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp
index 451df9ab7ee287..08037671a765c8 100644
--- a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp
+++ b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp
@@ -84,15 +84,9 @@ void test() {
Lock lock{mutex};
std::atomic_bool start = false;
- std::atomic_bool done = false;
auto thread = support::make_test_thread([&]() {
start.wait(false);
ss.request_stop();
-
- while (!done) {
- cv.notify_all();
- std::this_thread::sleep_for(std::chrono::milliseconds(2));
- }
});
std::same_as<bool> auto r = cv.wait(lock, ss.get_token(), [&]() {
@@ -101,7 +95,6 @@ void test() {
return false;
});
assert(!r);
- done = true;
thread.join();
assert(lock.owns_lock());
diff --git a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_token_pred.pass.cpp b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_token_pred.pass.cpp
index 6cdcbe36d98598..dcffa7aa0eb563 100644
--- a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_token_pred.pass.cpp
+++ b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_token_pred.pass.cpp
@@ -134,15 +134,9 @@ void test() {
Lock lock{mutex};
std::atomic_bool start = false;
- std::atomic_bool done = false;
auto thread = support::make_test_thread([&]() {
start.wait(false);
ss.request_stop();
-
- while (!done) {
- cv.notify_all();
- std::this_thread::sleep_for(std::chrono::milliseconds(2));
- }
});
std::same_as<bool> auto r = cv.wait_until(lock, ss.get_token(), future, [&]() {
@@ -151,7 +145,6 @@ void test() {
return false;
});
assert(!r);
- done = true;
thread.join();
assert(lock.owns_lock());
More information about the libcxx-commits
mailing list