[libcxx-commits] [libcxx] [libc++] fix condition_variable_any hangs on stop_request (PR #77127)
via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jan 9 10:02:39 PST 2024
https://github.com/huixie90 updated https://github.com/llvm/llvm-project/pull/77127
>From a497c77e9afd5883fda69d1e48bd4dd329e53d11 Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Fri, 5 Jan 2024 18:59:09 +0000
Subject: [PATCH 1/2] [libc++] fix condition_variable_any hangs on stop_request
---
libcxx/include/condition_variable | 3 +++
.../wait_for_token_pred.pass.cpp | 22 +++++++++++++++++++
.../wait_token_pred.pass.cpp | 22 +++++++++++++++++++
.../wait_until_token_pred.pass.cpp | 22 +++++++++++++++++++
4 files changed, 69 insertions(+)
diff --git a/libcxx/include/condition_variable b/libcxx/include/condition_variable
index cf7a570b6cb635..c512901cfb19ca 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,6 +258,7 @@ 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) {
+ stop_callback __cb(__stoken, [this] { notify_all(); });
while (!__stoken.stop_requested()) {
if (__pred())
return true;
@@ -268,6 +270,7 @@ bool condition_variable_any::wait(_Lock& __lock, stop_token __stoken, _Predicate
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) {
+ stop_callback __cb(__stoken, [this] { notify_all(); });
while (!__stoken.stop_requested()) {
if (__pred())
return true;
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..4bb089f3d2858f 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
@@ -155,6 +155,28 @@ void test() {
assert(lock.owns_lock());
}
+ // #76807 Hangs in std::condition_variable_any when used with std::stop_token
+ {
+ class MyThread {
+ public:
+ MyThread() {
+ thread_ = support::make_test_jthread([this](std::stop_token st) {
+ while (!st.stop_requested()) {
+ std::unique_lock lock{m_};
+ cv_.wait_for(lock, st, 1h, [] { return false; });
+ }
+ });
+ }
+
+ private:
+ std::mutex m_;
+ std::condition_variable_any cv_;
+ std::jthread thread_;
+ };
+
+ [[maybe_unused]] MyThread my_thread;
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
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..15a64a141c6d0d 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
@@ -107,6 +107,28 @@ void test() {
assert(lock.owns_lock());
}
+ // #76807 Hangs in std::condition_variable_any when used with std::stop_token
+ {
+ class MyThread {
+ public:
+ MyThread() {
+ thread_ = support::make_test_jthread([this](std::stop_token st) {
+ while (!st.stop_requested()) {
+ std::unique_lock lock{m_};
+ cv_.wait(lock, st, [] { return false; });
+ }
+ });
+ }
+
+ private:
+ std::mutex m_;
+ std::condition_variable_any cv_;
+ std::jthread thread_;
+ };
+
+ [[maybe_unused]] MyThread my_thread;
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
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..3d6f0d3eae25a6 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
@@ -157,6 +157,28 @@ void test() {
assert(lock.owns_lock());
}
+ // #76807 Hangs in std::condition_variable_any when used with std::stop_token
+ {
+ class MyThread {
+ public:
+ MyThread() {
+ thread_ = support::make_test_jthread([this](std::stop_token st) {
+ while (!st.stop_requested()) {
+ std::unique_lock lock{m_};
+ cv_.wait_until(lock, st, std::chrono::steady_clock::now() + std::chrono::hours(1), [] { return false; });
+ }
+ });
+ }
+
+ private:
+ std::mutex m_;
+ std::condition_variable_any cv_;
+ std::jthread thread_;
+ };
+
+ [[maybe_unused]] MyThread my_thread;
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
>From 52ffe6ecc5ebc52771c76ffdbc8148646e867197 Mon Sep 17 00:00:00 2001
From: Hui <hui.xie0621 at gmail.com>
Date: Tue, 9 Jan 2024 18:02:26 +0000
Subject: [PATCH 2/2] fix stop_requested lock issue
---
libcxx/include/condition_variable | 76 ++++++++++++++-----
.../wait_for_token_pred.pass.cpp | 26 +++++++
.../wait_token_pred.pass.cpp | 26 +++++++
.../wait_until_token_pred.pass.cpp | 26 +++++++
4 files changed, 136 insertions(+), 18 deletions(-)
diff --git a/libcxx/include/condition_variable b/libcxx/include/condition_variable
index c512901cfb19ca..e6399155d87edd 100644
--- a/libcxx/include/condition_variable
+++ b/libcxx/include/condition_variable
@@ -126,7 +126,6 @@ public:
#include <__condition_variable/condition_variable.h>
#include <__config>
#include <__memory/shared_ptr.h>
-#include <__memory/unique_ptr.h>
#include <__mutex/lock_guard.h>
#include <__mutex/mutex.h>
#include <__mutex/tag_types.h>
@@ -201,19 +200,26 @@ inline void condition_variable_any::notify_all() _NOEXCEPT {
__cv_.notify_all();
}
-struct __lock_external {
- template <class _Lock>
- _LIBCPP_HIDE_FROM_ABI void operator()(_Lock* __m) {
- __m->lock();
+template <class _Lock>
+struct __unlock_guard {
+ _Lock& __lock_;
+
+ _LIBCPP_HIDE_FROM_ABI __unlock_guard(_Lock& __lock) : __lock_(__lock) { __lock_.unlock(); }
+
+ _LIBCPP_HIDE_FROM_ABI ~__unlock_guard() noexcept // turns exception to std::terminate
+ {
+ __lock_.lock();
}
+
+ __unlock_guard(const __unlock_guard&) = delete;
+ __unlock_guard& operator=(const __unlock_guard&) = delete;
};
template <class _Lock>
void condition_variable_any::wait(_Lock& __lock) {
shared_ptr<mutex> __mut = __mut_;
unique_lock<mutex> __lk(*__mut);
- __lock.unlock();
- unique_ptr<_Lock, __lock_external> __lxx(&__lock);
+ __unlock_guard<_Lock> __unlock(__lock);
lock_guard<unique_lock<mutex> > __lx(__lk, adopt_lock_t());
__cv_.wait(__lk);
} // __mut_.unlock(), __lock.lock()
@@ -228,8 +234,7 @@ template <class _Lock, class _Clock, class _Duration>
cv_status condition_variable_any::wait_until(_Lock& __lock, const chrono::time_point<_Clock, _Duration>& __t) {
shared_ptr<mutex> __mut = __mut_;
unique_lock<mutex> __lk(*__mut);
- __lock.unlock();
- unique_ptr<_Lock, __lock_external> __lxx(&__lock);
+ __unlock_guard<_Lock> __unlock(__lock);
lock_guard<unique_lock<mutex> > __lx(__lk, adopt_lock_t());
return __cv_.wait_until(__lk, __t);
} // __mut_.unlock(), __lock.lock()
@@ -257,25 +262,60 @@ condition_variable_any::wait_for(_Lock& __lock, const chrono::duration<_Rep, _Pe
# if _LIBCPP_STD_VER >= 20 && !defined(_LIBCPP_HAS_NO_EXPERIMENTAL_STOP_TOKEN)
template <class _Lock, class _Predicate>
-bool condition_variable_any::wait(_Lock& __lock, stop_token __stoken, _Predicate __pred) {
- stop_callback __cb(__stoken, [this] { notify_all(); });
- while (!__stoken.stop_requested()) {
+bool condition_variable_any::wait(_Lock& __user_lock, stop_token __stoken, _Predicate __pred) {
+ // It is not safe to call the destructor on the other thread while waiting,
+ // even if we copy the shared_ptr<mutex> inside the function,
+ // because the stop_callback needs the internal cv.
+ // In order to make it work, we need to change the ABI to make both
+ // mutex and internal cv to be hold inside the shared_ptr, and
+ // make a local copy inside this function
+
+ if (__stoken.stop_requested())
+ return __pred();
+ std::stop_callback __cb(__stoken, [this] { notify_all(); });
+
+ while (true) {
if (__pred())
return true;
- wait(__lock);
- }
+
+ // We need to take the internal lock before checking stop_requested,
+ // so that the notification cannot come in between the stop_requested
+ // check and entering the wait.
+ // Note that the stop_callback takes the same internal lock before notifying
+ unique_lock<mutex> __internal_lock(*__mut_);
+ if (__stoken.stop_requested())
+ break;
+
+ __unlock_guard<_Lock> __unlock(__user_lock);
+ unique_lock<mutex> __internal_lock2(std::move(__internal_lock));
+ __cv_.wait(__internal_lock2);
+ } // __mut_.unlock(), __lock.lock()
return __pred();
}
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) {
+ _Lock& __user_lock,
+ stop_token __stoken,
+ const chrono::time_point<_Clock, _Duration>& __abs_time,
+ _Predicate __pred) {
+ if (__stoken.stop_requested())
+ return __pred();
stop_callback __cb(__stoken, [this] { notify_all(); });
- while (!__stoken.stop_requested()) {
+
+ while (true) {
if (__pred())
return true;
- if (wait_until(__lock, __abs_time) == cv_status::timeout)
- return __pred();
+
+ unique_lock<mutex> __internal_lock(*__mut_);
+ if (__stoken.stop_requested())
+ break;
+
+ __unlock_guard<_Lock> __unlock(__user_lock);
+ unique_lock<mutex> __internal_lock2(std::move(__internal_lock));
+
+ if (__cv_.wait_until(__internal_lock2, __abs_time) == cv_status::timeout)
+ break;
}
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 4bb089f3d2858f..d06576d7c3dd09 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
@@ -177,6 +177,32 @@ void test() {
[[maybe_unused]] MyThread my_thread;
}
+ // request_stop potentially in-between check and wait
+ {
+ std::stop_source ss;
+ std::condition_variable_any cv;
+ Mutex mutex;
+ Lock lock{mutex};
+
+ std::atomic_bool pred_started = false;
+ std::atomic_bool request_stop_called = false;
+ auto thread = support::make_test_thread([&]() {
+ pred_started.wait(false);
+ ss.request_stop();
+ request_stop_called.store(true);
+ });
+
+ std::same_as<bool> auto r = cv.wait_for(lock, ss.get_token(), 1h, [&]() {
+ pred_started.store(true);
+ request_stop_called.wait(false);
+ return false;
+ });
+ assert(!r);
+ thread.join();
+
+ assert(lock.owns_lock());
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
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 15a64a141c6d0d..7dceb4e8eb73cb 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
@@ -129,6 +129,32 @@ void test() {
[[maybe_unused]] MyThread my_thread;
}
+ // request_stop potentially in-between check and wait
+ {
+ std::stop_source ss;
+ std::condition_variable_any cv;
+ Mutex mutex;
+ Lock lock{mutex};
+
+ std::atomic_bool pred_started = false;
+ std::atomic_bool request_stop_called = false;
+ auto thread = support::make_test_thread([&]() {
+ pred_started.wait(false);
+ ss.request_stop();
+ request_stop_called.store(true);
+ });
+
+ std::same_as<bool> auto r = cv.wait(lock, ss.get_token(), [&]() {
+ pred_started.store(true);
+ request_stop_called.wait(false);
+ return false;
+ });
+ assert(!r);
+ thread.join();
+
+ assert(lock.owns_lock());
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
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 3d6f0d3eae25a6..95371aa395f4e6 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
@@ -179,6 +179,32 @@ void test() {
[[maybe_unused]] MyThread my_thread;
}
+ // request_stop potentially in-between check and wait
+ {
+ std::stop_source ss;
+ std::condition_variable_any cv;
+ Mutex mutex;
+ Lock lock{mutex};
+
+ std::atomic_bool pred_started = false;
+ std::atomic_bool request_stop_called = false;
+ auto thread = support::make_test_thread([&]() {
+ pred_started.wait(false);
+ ss.request_stop();
+ request_stop_called.store(true);
+ });
+
+ std::same_as<bool> auto r = cv.wait_until(lock, ss.get_token(), future, [&]() {
+ pred_started.store(true);
+ request_stop_called.wait(false);
+ return false;
+ });
+ assert(!r);
+ thread.join();
+
+ assert(lock.owns_lock());
+ }
+
#if !defined(TEST_HAS_NO_EXCEPTIONS)
// Throws: Any exception thrown by pred.
{
More information about the libcxx-commits
mailing list