[libcxx-commits] [libcxx] 85a8e5c - [libc++] fix condition_variable_any hangs on stop_request (#77127)
via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Jan 20 13:18:49 PST 2024
Author: Hui
Date: 2024-01-20T21:18:44Z
New Revision: 85a8e5c3e0586e85a2fa3ff9cef12455bd039921
URL: https://github.com/llvm/llvm-project/commit/85a8e5c3e0586e85a2fa3ff9cef12455bd039921
DIFF: https://github.com/llvm/llvm-project/commit/85a8e5c3e0586e85a2fa3ff9cef12455bd039921.diff
LOG: [libc++] fix condition_variable_any hangs on stop_request (#77127)
When I implemented `condition_variable_any::wait`, I missed the most
important paragraph in the spec:
> The following wait functions will be notified when there is a stop
request on the passed stop_token.
> In that case the functions return immediately, returning false if the
predicate evaluates to false.
From
https://eel.is/c++draft/thread.condition#thread.condvarany.intwait-1.
Fixes #76807
Added:
libcxx/test/std/thread/thread.condition/thread.condition.condvarany/helpers.h
Modified:
libcxx/include/condition_variable
libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_token_pred.pass.cpp
libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_token_pred.pass.cpp
libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_token_pred.pass.cpp
Removed:
################################################################################
diff --git a/libcxx/include/condition_variable b/libcxx/include/condition_variable
index cf7a570b6cb635..e375c986e7f12e 100644
--- a/libcxx/include/condition_variable
+++ b/libcxx/include/condition_variable
@@ -126,11 +126,11 @@ 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>
#include <__mutex/unique_lock.h>
+#include <__stop_token/stop_callback.h>
#include <__stop_token/stop_token.h>
#include <__utility/move.h>
#include <version>
@@ -200,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()
@@ -227,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()
@@ -256,24 +262,75 @@ 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) {
- while (!__stoken.stop_requested()) {
+bool condition_variable_any::wait(_Lock& __user_lock, stop_token __stoken, _Predicate __pred) {
+ if (__stoken.stop_requested())
+ return __pred();
+
+ // Per https://eel.is/c++draft/thread.condition.condvarany#general-note-2,
+ // we do need to take a copy of the shared pointer __mut_
+ // This ensures that a thread can call the destructor immediately after calling
+ // notify_all, without waiting all the wait calls.
+ // A thread can also safely call the destructor immediately after calling
+ // request_stop, as the call to request_stop would evaluate the callback,
+ // which accesses the internal condition variable, immediately on the same thread.
+ // In this situation, it is OK even without copying a shared ownership the internal
+ // condition variable. However, this needs the evaluation of stop_callback to
+ // happen-before the destruction.
+ // The spec only says "Only the notification to unblock the wait needs to happen
+ // before destruction". To make this work, we need to copy the shared ownership of
+ // the internal condition variable inside this function, which is not possible
+ // with the current ABI.
+ shared_ptr<mutex> __mut = __mut_;
+
+ 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)); // switch unlock order between __internal_lock and __user_lock
+ __cv_.wait(__internal_lock2);
+ } // __internal_lock2.unlock(), __user_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) {
- while (!__stoken.stop_requested()) {
+ _Lock& __user_lock,
+ stop_token __stoken,
+ const chrono::time_point<_Clock, _Duration>& __abs_time,
+ _Predicate __pred) {
+ if (__stoken.stop_requested())
+ return __pred();
+
+ 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)
- 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)); // switch unlock order between __internal_lock and __user_lock
+
+ if (__cv_.wait_until(__internal_lock2, __abs_time) == cv_status::timeout)
+ break;
+ } // __internal_lock2.unlock(), __user_lock.lock()
return __pred();
}
diff --git a/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/helpers.h b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/helpers.h
new file mode 100644
index 00000000000000..83687a1c648c9f
--- /dev/null
+++ b/libcxx/test/std/thread/thread.condition/thread.condition.condvarany/helpers.h
@@ -0,0 +1,45 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TEST_STD_THREAD_CONDITION_CONDVARANY_HELPERS_H
+#define TEST_STD_THREAD_CONDITION_CONDVARANY_HELPERS_H
+
+#include <chrono>
+#include <cassert>
+
+#include "test_macros.h"
+
+#if TEST_STD_VER >= 17
+
+// wait_for and wait_until function can exit via
+// - predicate is true
+// - timeout
+// - stop_requested
+// The return value only tells if the predicate is true
+// when the function exits, but it does not tell whether
+// it is timeout or stop_requested.
+//
+// ElapsedTimeCheck would fail the test if a test takes
+// longer than a duration. This is useful because we can
+// ensure that the wait_{for, until} function does not
+// wait until the timeout
+struct ElapsedTimeCheck {
+ ElapsedTimeCheck(std::chrono::steady_clock::duration timeoutDuration)
+ : timeout_(std::chrono::steady_clock::now() + timeoutDuration) {}
+
+ ElapsedTimeCheck(ElapsedTimeCheck&&) = delete;
+ ElapsedTimeCheck& operator=(ElapsedTimeCheck&&) = delete;
+
+ ~ElapsedTimeCheck() { assert(std::chrono::steady_clock::now() < timeout_); }
+
+ std::chrono::time_point<std::chrono::steady_clock> timeout_;
+};
+
+#endif
+
+#endif // TEST_STD_THREAD_CONDITION_CONDVARANY_HELPERS_H
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..4ea60557d9f88c 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
@@ -29,6 +29,7 @@
#include <stop_token>
#include <thread>
+#include "helpers.h"
#include "make_test_thread.h"
#include "test_macros.h"
@@ -44,6 +45,8 @@ void test() {
Lock lock{mutex};
ss.request_stop();
+ ElapsedTimeCheck check(1min);
+
// [Note 4: The returned value indicates whether the predicate evaluated to true
// regardless of whether the timeout was triggered or a stop request was made.]
std::same_as<bool> auto r1 = cv.wait_for(lock, ss.get_token(), -1h, []() { return false; });
@@ -69,6 +72,8 @@ void test() {
Mutex mutex;
Lock lock{mutex};
+ ElapsedTimeCheck check(1min);
+
std::same_as<bool> auto r1 = cv.wait_for(lock, ss.get_token(), -1h, []() { return true; });
assert(r1);
@@ -83,6 +88,8 @@ void test() {
Mutex mutex;
Lock lock{mutex};
+ ElapsedTimeCheck check(1min);
+
std::same_as<bool> auto r1 = cv.wait_for(lock, ss.get_token(), -1h, []() { return false; });
assert(!r1);
}
@@ -117,6 +124,8 @@ void test() {
cv.notify_all();
});
+ ElapsedTimeCheck check(10min);
+
std::same_as<bool> auto r1 = cv.wait_for(lock, ss.get_token(), 1h, [&]() { return flag; });
assert(flag);
assert(r1);
@@ -143,6 +152,8 @@ void test() {
}
});
+ ElapsedTimeCheck check(10min);
+
std::same_as<bool> auto r = cv.wait_for(lock, ss.get_token(), 1h, [&]() {
start.store(true);
start.notify_all();
@@ -155,6 +166,60 @@ 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_;
+ };
+
+ ElapsedTimeCheck check(10min);
+
+ [[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);
+ request_stop_called.notify_all();
+ });
+
+ ElapsedTimeCheck check(10min);
+
+ std::same_as<bool> auto r = cv.wait_for(lock, ss.get_token(), 1h, [&]() {
+ pred_started.store(true);
+ pred_started.notify_all();
+ 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.
{
@@ -164,6 +229,7 @@ void test() {
Lock lock{mutex};
try {
+ ElapsedTimeCheck check(10min);
cv.wait_for(lock, ss.get_token(), 1h, []() -> bool { throw 5; });
assert(false);
} catch (int i) {
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..e96a3e8bd1bc0b 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,56 @@ 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;
+ }
+
+ // 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);
+ request_stop_called.notify_all();
+ });
+
+ std::same_as<bool> auto r = cv.wait(lock, ss.get_token(), [&]() {
+ pred_started.store(true);
+ pred_started.notify_all();
+ 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 6cdcbe36d98598..d649db025d755d 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
@@ -29,13 +29,15 @@
#include <stop_token>
#include <thread>
+#include "helpers.h"
#include "make_test_thread.h"
#include "test_macros.h"
template <class Mutex, class Lock>
void test() {
- const auto past = std::chrono::steady_clock::now() - std::chrono::hours(1);
- const auto future = std::chrono::steady_clock::now() + std::chrono::hours(1);
+ using namespace std::chrono_literals;
+ const auto oneHourAgo = std::chrono::steady_clock::now() - 1h;
+ const auto oneHourLater = std::chrono::steady_clock::now() + 1h;
// stop_requested before hand
{
@@ -44,19 +46,20 @@ void test() {
Mutex mutex;
Lock lock{mutex};
ss.request_stop();
+ ElapsedTimeCheck check(1min);
// [Note 4: The returned value indicates whether the predicate evaluated to true
// regardless of whether the timeout was triggered or a stop request was made.]
- std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), past, []() { return false; });
+ std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), oneHourAgo, []() { return false; });
assert(!r1);
- std::same_as<bool> auto r2 = cv.wait_until(lock, ss.get_token(), future, []() { return false; });
+ std::same_as<bool> auto r2 = cv.wait_until(lock, ss.get_token(), oneHourLater, []() { return false; });
assert(!r2);
- std::same_as<bool> auto r3 = cv.wait_until(lock, ss.get_token(), past, []() { return true; });
+ std::same_as<bool> auto r3 = cv.wait_until(lock, ss.get_token(), oneHourAgo, []() { return true; });
assert(r3);
- std::same_as<bool> auto r4 = cv.wait_until(lock, ss.get_token(), future, []() { return true; });
+ std::same_as<bool> auto r4 = cv.wait_until(lock, ss.get_token(), oneHourLater, []() { return true; });
assert(r4);
// Postconditions: lock is locked by the calling thread.
@@ -69,11 +72,12 @@ void test() {
std::condition_variable_any cv;
Mutex mutex;
Lock lock{mutex};
+ ElapsedTimeCheck check(1min);
- std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), past, []() { return true; });
+ std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), oneHourAgo, []() { return true; });
assert(r1);
- std::same_as<bool> auto r2 = cv.wait_until(lock, ss.get_token(), future, []() { return true; });
+ std::same_as<bool> auto r2 = cv.wait_until(lock, ss.get_token(), oneHourLater, []() { return true; });
assert(r2);
}
@@ -83,8 +87,9 @@ void test() {
std::condition_variable_any cv;
Mutex mutex;
Lock lock{mutex};
+ ElapsedTimeCheck check(1min);
- std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), past, []() { return false; });
+ std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), oneHourAgo, []() { return false; });
assert(!r1);
}
@@ -119,7 +124,9 @@ void test() {
cv.notify_all();
});
- std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), future, [&]() { return flag; });
+ ElapsedTimeCheck check(10min);
+
+ std::same_as<bool> auto r1 = cv.wait_until(lock, ss.get_token(), oneHourLater, [&]() { return flag; });
assert(flag);
assert(r1);
@@ -145,7 +152,9 @@ void test() {
}
});
- std::same_as<bool> auto r = cv.wait_until(lock, ss.get_token(), future, [&]() {
+ ElapsedTimeCheck check(10min);
+
+ std::same_as<bool> auto r = cv.wait_until(lock, ss.get_token(), oneHourLater, [&]() {
start.store(true);
start.notify_all();
return false;
@@ -157,6 +166,60 @@ 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_;
+ };
+
+ ElapsedTimeCheck check(10min);
+
+ [[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);
+ request_stop_called.notify_all();
+ });
+
+ ElapsedTimeCheck check(10min);
+
+ std::same_as<bool> auto r = cv.wait_until(lock, ss.get_token(), oneHourLater, [&]() {
+ pred_started.store(true);
+ pred_started.notify_all();
+ 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.
{
@@ -166,7 +229,7 @@ void test() {
Lock lock{mutex};
try {
- cv.wait_until(lock, ss.get_token(), future, []() -> bool { throw 5; });
+ cv.wait_until(lock, ss.get_token(), oneHourLater, []() -> bool { throw 5; });
assert(false);
} catch (int i) {
assert(i == 5);
More information about the libcxx-commits
mailing list