diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index 8d52f7265bf..44d381e4150 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -90,7 +90,7 @@ public: } template - cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) { + cv_status wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time) { // wait until time point static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required"); const auto _Now = _Clock::now(); @@ -100,7 +100,7 @@ public: } template - bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) { + bool wait_until(_Lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) { // wait for signal with timeout and check predicate #if _HAS_CXX20 static_assert(chrono::is_clock_v<_Clock>, "Clock type required"); @@ -115,7 +115,7 @@ public: } template - cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) { // wait for duration + cv_status wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time) { // wait for duration if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) { _Unlock_guard<_Lock> _Unlock_outer{_Lck}; (void) _Unlock_outer; @@ -146,7 +146,7 @@ public: } template - bool wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) { + bool wait_for(_Lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) { // wait for signal with timeout and check predicate return wait_until(_Lck, _To_absolute_time(_Rel_time), _STD move(_Pred)); } @@ -194,7 +194,7 @@ public: template bool wait_until( - _Lock& _Lck, stop_token _Stoken, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) { + _Lock& _Lck, stop_token _Stoken, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) { static_assert(chrono::is_clock_v<_Clock>, "Clock type required"); stop_callback<_Cv_any_notify_all> _Cb{_Stoken, this}; for (;;) { @@ -224,7 +224,7 @@ public: } template - bool wait_for(_Lock& _Lck, stop_token _Stoken, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) { + bool wait_for(_Lock& _Lck, stop_token _Stoken, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) { return wait_until(_Lck, _STD move(_Stoken), _To_absolute_time(_Rel_time), _STD move(_Pred)); } #endif // _HAS_CXX20 diff --git a/stl/inc/mutex b/stl/inc/mutex index 8cb450c59d7..0deda247d62 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -556,7 +556,7 @@ public: } template - cv_status wait_for(unique_lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) { + cv_status wait_for(unique_lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time) { // wait for duration if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) { // we don't unlock-and-relock _Lck for this case because it's not observable @@ -566,13 +566,13 @@ public: } template - bool wait_for(unique_lock& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) { + bool wait_for(unique_lock& _Lck, const chrono::duration<_Rep, _Period> _Rel_time, _Predicate _Pred) { // wait for signal with timeout and check predicate return wait_until(_Lck, _To_absolute_time(_Rel_time), _STD _Pass_fn(_Pred)); } template - cv_status wait_until(unique_lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time) { + cv_status wait_until(unique_lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time) { // wait until time point static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required"); #if _ITERATOR_DEBUG_LEVEL != 0 @@ -598,8 +598,7 @@ public: } template - bool wait_until( - unique_lock& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate _Pred) { + bool wait_until(unique_lock& _Lck, const chrono::time_point<_Clock, _Duration> _Abs_time, _Predicate _Pred) { // wait for signal with timeout and check predicate static_assert(chrono::_Is_clock_v<_Clock>, "Clock type required"); while (!_Pred()) { diff --git a/tests/std/tests/GH_000685_condition_variable_any/test.cpp b/tests/std/tests/GH_000685_condition_variable_any/test.cpp index 03ebd7023b8..1edf99b2c3b 100644 --- a/tests/std/tests/GH_000685_condition_variable_any/test.cpp +++ b/tests/std/tests/GH_000685_condition_variable_any/test.cpp @@ -2,13 +2,21 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // Test GH-685 "wait_for in condition_variable_any should unlock and lock" +// Test LWG-4301 "condition_variable{_any}::wait_{for, until} should take timeout by value" +#include #include #include #include +#include #include +#include #include +#if _HAS_CXX20 +#include +#endif // _HAS_CXX20 + using namespace std; using namespace std::chrono; @@ -79,9 +87,135 @@ namespace { assert(m.num_locks() == 4); #endif // _HAS_CXX20 } + + // Minimal example inspired by LWG-4301, modified due to missing std::latch before C++20 + // and generalized to test all overloads of condition_variable{_any}::wait_{for, until}. + // Idea: Make the main thread wait for a CV with a short timeout and modify it from another thread in the meantime. + // If the main thread wait times out after a short time, the modification did not influence the ongoing wait. + template + void test_timeout_immutable(const int test_number, const int retries_remaining = 5) { + printf("\ntest %d\n", test_number); + + mutex m; + CV cv; + unique_lock main_lock(m); // Prevent other thread from modifying timeout too early + + // Start with very short timeout and let other_thread change it to very large while main thread is waiting + constexpr auto short_timeout = 1s; + constexpr auto long_timeout = 10s; + + atomic_flag waiting_for_other_thread{}; + waiting_for_other_thread.test_and_set(); + + const auto wait_start = steady_clock::now(); + auto timeout_duration = short_timeout; + auto timeout = wait_start + timeout_duration; + + const auto set_timeout = [&](const auto new_timeout) { + timeout_duration = new_timeout; + timeout = steady_clock::now() + new_timeout; + }; + + thread other_thread([&] { + printf( + "thread start after %lld ms\n", duration_cast(steady_clock::now() - wait_start).count()); + waiting_for_other_thread.clear(); + // Immediately blocks since the main thread owns the mutex m. + lock_guard other_lock(m); + puts("thread lock"); + + // If the timeout provided to condition_variable{_any}::wait_{for, until} was mutable, + // we will get timeout in the main thread after much longer time + set_timeout(long_timeout); + puts("thread end"); + }); + + while (waiting_for_other_thread.test_and_set()) { + this_thread::yield(); // freeze the main thread from proceeding until other thread is started + } + printf("main resumed after %lld ms\n", duration_cast(steady_clock::now() - wait_start).count()); + set_timeout(short_timeout); + + puts("main waiting"); + const bool cv_wait_timed_out = [&] { + switch (test_number) { + case 0: + return cv.wait_until(main_lock, timeout) == cv_status::timeout; + + case 1: + return cv.wait_until(main_lock, timeout, [] { return false; }) == false; + + case 2: + return cv.wait_for(main_lock, timeout_duration) == cv_status::timeout; + + case 3: + return cv.wait_for(main_lock, timeout_duration, [] { return false; }) == false; + +#if _HAS_CXX20 // because of stop_token + case 4: + if constexpr (is_same_v) { + stop_source source; + return cv.wait_until(main_lock, source.get_token(), timeout, [] { return false; }) == false; + } else { + assert(false); // test not supported for std::condition_variable + return false; + } + + case 5: + if constexpr (is_same_v) { + stop_source source; + return cv.wait_for(main_lock, source.get_token(), timeout_duration, [] { return false; }) == false; + } else { + assert(false); // test not supported for std::condition_variable + return false; + } +#endif // _HAS_CXX20 + + default: + assert(false); + return false; + } + }(); + + const auto elapsed = steady_clock::now() - wait_start; + + if (!cv_wait_timed_out) { + if (retries_remaining > 0) { + printf("unexpected wakeup after %lld ms, retry %d...\n", duration_cast(elapsed).count(), + retries_remaining); + test_timeout_immutable(test_number, retries_remaining - 1); // recurse to try the test again + } else { + puts("Too many unexpected wakeups"); + assert(false); + } + } else { + assert(elapsed < long_timeout / 2); + printf("wait end after %lld ms\n", duration_cast(elapsed).count()); + } + + // Make sure the child thread has indeed finished (so the next join does not block) + assert(timeout_duration == long_timeout); + other_thread.join(); + } } // unnamed namespace int main() { test_condition_variable_any(); test_condition_variable_any_already_timed_out(); + + puts("condition_variable"); + test_timeout_immutable(0); + test_timeout_immutable(1); + test_timeout_immutable(2); + test_timeout_immutable(3); + + puts("condition_variable_any"); + test_timeout_immutable(0); + test_timeout_immutable(1); + test_timeout_immutable(2); + test_timeout_immutable(3); +#if _HAS_CXX20 + test_timeout_immutable(4); + test_timeout_immutable(5); +#endif // _HAS_CXX20 }