Skip to content

Commit 2725c36

Browse files
Implement LWG-4301: condition_variable{_any}::wait_{for, until} should take timeout by value (#5885)
Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>
1 parent 20e274b commit 2725c36

File tree

3 files changed

+144
-11
lines changed

3 files changed

+144
-11
lines changed

stl/inc/condition_variable

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public:
9090
}
9191

9292
template <class _Lock, class _Clock, class _Duration>
93-
cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) {
93+
cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time) {
9494
// wait until time point
9595
static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required");
9696
const auto _Now = _Clock::now();
@@ -100,7 +100,7 @@ public:
100100
}
101101

102102
template <class _Lock, class _Clock, class _Duration, class _Predicate>
103-
bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) {
103+
bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) {
104104
// wait for signal with timeout and check predicate
105105
#if _HAS_CXX20
106106
static_assert(chrono::is_clock_v<_Clock>, "Clock type required");
@@ -115,7 +115,7 @@ public:
115115
}
116116

117117
template <class _Lock, class _Rep, class _Period>
118-
cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) { // wait for duration
118+
cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time) { // wait for duration
119119
if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) {
120120
_Unlock_guard<_Lock> _Unlock_outer{_Lck};
121121
(void) _Unlock_outer;
@@ -146,7 +146,7 @@ public:
146146
}
147147

148148
template <class _Lock, class _Rep, class _Period, class _Predicate>
149-
bool wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) {
149+
bool wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) {
150150
// wait for signal with timeout and check predicate
151151
return wait_until(_Lck, _To_absolute_time(_Rel_time), _STD move(_Pred));
152152
}
@@ -194,7 +194,7 @@ public:
194194

195195
template <class _Lock, class _Clock, class _Duration, class _Predicate>
196196
bool wait_until(
197-
_Lock& _Lck, stop_token _Stoken, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) {
197+
_Lock& _Lck, stop_token _Stoken, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) {
198198
static_assert(chrono::is_clock_v<_Clock>, "Clock type required");
199199
stop_callback<_Cv_any_notify_all> _Cb{_Stoken, this};
200200
for (;;) {
@@ -224,7 +224,7 @@ public:
224224
}
225225

226226
template <class _Lock, class _Rep, class _Period, class _Predicate>
227-
bool wait_for(_Lock& _Lck, stop_token _Stoken, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) {
227+
bool wait_for(_Lock& _Lck, stop_token _Stoken, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) {
228228
return wait_until(_Lck, _STD move(_Stoken), _To_absolute_time(_Rel_time), _STD move(_Pred));
229229
}
230230
#endif // _HAS_CXX20

stl/inc/mutex

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,7 @@ public:
556556
}
557557

558558
template <class _Rep, class _Period>
559-
cv_status wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) {
559+
cv_status wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period> _Rel_time) {
560560
// wait for duration
561561
if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) {
562562
// we don't unlock-and-relock _Lck for this case because it's not observable
@@ -566,13 +566,13 @@ public:
566566
}
567567

568568
template <class _Rep, class _Period, class _Predicate>
569-
bool wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) {
569+
bool wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) {
570570
// wait for signal with timeout and check predicate
571571
return wait_until(_Lck, _To_absolute_time(_Rel_time), _STD _Pass_fn(_Pred));
572572
}
573573

574574
template <class _Clock, class _Duration>
575-
cv_status wait_until(unique_lock<mutex>& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) {
575+
cv_status wait_until(unique_lock<mutex>& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time) {
576576
// wait until time point
577577
static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required");
578578
#if _ITERATOR_DEBUG_LEVEL != 0
@@ -598,8 +598,7 @@ public:
598598
}
599599

600600
template <class _Clock, class _Duration, class _Predicate>
601-
bool wait_until(
602-
unique_lock<mutex>& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) {
601+
bool wait_until(unique_lock<mutex>& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) {
603602
// wait for signal with timeout and check predicate
604603
static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required");
605604
while (!_Pred()) {

tests/std/tests/GH_000685_condition_variable_any/test.cpp

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,21 @@
22
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
33

44
// Test GH-685 "wait_for in condition_variable_any should unlock and lock"
5+
// Test LWG-4301 "condition_variable{_any}::wait_{for, until} should take timeout by value"
56

7+
#include <atomic>
68
#include <cassert>
79
#include <chrono>
810
#include <condition_variable>
11+
#include <cstdio>
912
#include <mutex>
13+
#include <thread>
1014
#include <type_traits>
1115

16+
#if _HAS_CXX20
17+
#include <stop_token>
18+
#endif // _HAS_CXX20
19+
1220
using namespace std;
1321
using namespace std::chrono;
1422

@@ -79,9 +87,135 @@ namespace {
7987
assert(m.num_locks() == 4);
8088
#endif // _HAS_CXX20
8189
}
90+
91+
// Minimal example inspired by LWG-4301, modified due to missing std::latch before C++20
92+
// and generalized to test all overloads of condition_variable{_any}::wait_{for, until}.
93+
// Idea: Make the main thread wait for a CV with a short timeout and modify it from another thread in the meantime.
94+
// If the main thread wait times out after a short time, the modification did not influence the ongoing wait.
95+
template <typename CV>
96+
void test_timeout_immutable(const int test_number, const int retries_remaining = 5) {
97+
printf("\ntest %d\n", test_number);
98+
99+
mutex m;
100+
CV cv;
101+
unique_lock<mutex> main_lock(m); // Prevent other thread from modifying timeout too early
102+
103+
// Start with very short timeout and let other_thread change it to very large while main thread is waiting
104+
constexpr auto short_timeout = 1s;
105+
constexpr auto long_timeout = 10s;
106+
107+
atomic_flag waiting_for_other_thread{};
108+
waiting_for_other_thread.test_and_set();
109+
110+
const auto wait_start = steady_clock::now();
111+
auto timeout_duration = short_timeout;
112+
auto timeout = wait_start + timeout_duration;
113+
114+
const auto set_timeout = [&](const auto new_timeout) {
115+
timeout_duration = new_timeout;
116+
timeout = steady_clock::now() + new_timeout;
117+
};
118+
119+
thread other_thread([&] {
120+
printf(
121+
"thread start after %lld ms\n", duration_cast<milliseconds>(steady_clock::now() - wait_start).count());
122+
waiting_for_other_thread.clear();
123+
// Immediately blocks since the main thread owns the mutex m.
124+
lock_guard<mutex> other_lock(m);
125+
puts("thread lock");
126+
127+
// If the timeout provided to condition_variable{_any}::wait_{for, until} was mutable,
128+
// we will get timeout in the main thread after much longer time
129+
set_timeout(long_timeout);
130+
puts("thread end");
131+
});
132+
133+
while (waiting_for_other_thread.test_and_set()) {
134+
this_thread::yield(); // freeze the main thread from proceeding until other thread is started
135+
}
136+
printf("main resumed after %lld ms\n", duration_cast<milliseconds>(steady_clock::now() - wait_start).count());
137+
set_timeout(short_timeout);
138+
139+
puts("main waiting");
140+
const bool cv_wait_timed_out = [&] {
141+
switch (test_number) {
142+
case 0:
143+
return cv.wait_until(main_lock, timeout) == cv_status::timeout;
144+
145+
case 1:
146+
return cv.wait_until(main_lock, timeout, [] { return false; }) == false;
147+
148+
case 2:
149+
return cv.wait_for(main_lock, timeout_duration) == cv_status::timeout;
150+
151+
case 3:
152+
return cv.wait_for(main_lock, timeout_duration, [] { return false; }) == false;
153+
154+
#if _HAS_CXX20 // because of stop_token
155+
case 4:
156+
if constexpr (is_same_v<CV, condition_variable_any>) {
157+
stop_source source;
158+
return cv.wait_until(main_lock, source.get_token(), timeout, [] { return false; }) == false;
159+
} else {
160+
assert(false); // test not supported for std::condition_variable
161+
return false;
162+
}
163+
164+
case 5:
165+
if constexpr (is_same_v<CV, condition_variable_any>) {
166+
stop_source source;
167+
return cv.wait_for(main_lock, source.get_token(), timeout_duration, [] { return false; }) == false;
168+
} else {
169+
assert(false); // test not supported for std::condition_variable
170+
return false;
171+
}
172+
#endif // _HAS_CXX20
173+
174+
default:
175+
assert(false);
176+
return false;
177+
}
178+
}();
179+
180+
const auto elapsed = steady_clock::now() - wait_start;
181+
182+
if (!cv_wait_timed_out) {
183+
if (retries_remaining > 0) {
184+
printf("unexpected wakeup after %lld ms, retry %d...\n", duration_cast<milliseconds>(elapsed).count(),
185+
retries_remaining);
186+
test_timeout_immutable<CV>(test_number, retries_remaining - 1); // recurse to try the test again
187+
} else {
188+
puts("Too many unexpected wakeups");
189+
assert(false);
190+
}
191+
} else {
192+
assert(elapsed < long_timeout / 2);
193+
printf("wait end after %lld ms\n", duration_cast<milliseconds>(elapsed).count());
194+
}
195+
196+
// Make sure the child thread has indeed finished (so the next join does not block)
197+
assert(timeout_duration == long_timeout);
198+
other_thread.join();
199+
}
82200
} // unnamed namespace
83201

84202
int main() {
85203
test_condition_variable_any();
86204
test_condition_variable_any_already_timed_out();
205+
206+
puts("condition_variable");
207+
test_timeout_immutable<condition_variable>(0);
208+
test_timeout_immutable<condition_variable>(1);
209+
test_timeout_immutable<condition_variable>(2);
210+
test_timeout_immutable<condition_variable>(3);
211+
212+
puts("condition_variable_any");
213+
test_timeout_immutable<condition_variable_any>(0);
214+
test_timeout_immutable<condition_variable_any>(1);
215+
test_timeout_immutable<condition_variable_any>(2);
216+
test_timeout_immutable<condition_variable_any>(3);
217+
#if _HAS_CXX20
218+
test_timeout_immutable<condition_variable_any>(4);
219+
test_timeout_immutable<condition_variable_any>(5);
220+
#endif // _HAS_CXX20
87221
}

0 commit comments

Comments
 (0)