Message ID | cec397280e969c01f78eb32aec27ddd14f9f89bb.1532105301.git-series.mac@mcrowe.com |
---|---|
State | New |
Headers | show |
Series | [1/2] condition_variable: Report early wakeup of wait_until as no_timeout | expand |
On 20/07/18 17:49 +0100, Mike Crowe wrote: >As currently implemented, condition_variable always ultimately waits >against std::chrono::system_clock. This clock can be changed in arbitrary >ways by the user which may result in us waking up too early or too late >when measured against the caller-supplied clock. > >We can't (yet) do much about waking up too late[1], but >if we wake up too early we must return cv_status::no_timeout to indicate a >spurious wakeup rather than incorrectly returning cv_status::timeout. > >[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861 Committed to trunk, thanks very much. I reformatted it slightly, to keep the line below 80 columns. The version I committed is attached. commit 79a8b4c1d70287ac0e668c4f2335d70d97c3002e Author: redi <redi@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Wed Aug 1 15:39:45 2018 +0000 Report early wakeup of condition_variable::wait_until as no_timeout As currently implemented, condition_variable always ultimately waits against std::chrono::system_clock. This clock can be changed in arbitrary ways by the user which may result in us waking up too early or too late when measured against the caller-supplied clock. We can't (yet) do much about waking up too late (PR 41861), but if we wake up too early we must return cv_status::no_timeout to indicate a spurious wakeup rather than incorrectly returning cv_status::timeout. 2018-08-01 Mike Crowe <mac@mcrowe.com> * include/std/condition_variable (wait_until): Only report timeout if we really have timed out when measured against the caller-supplied clock. * testsuite/30_threads/condition_variable/members/2.cc: Add test case to confirm above behaviour. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@263224 138bc75d-0d04-0410-961f-82ee72b054a4 diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable index 3f690c81799..c00afa2b7ae 100644 --- a/libstdc++-v3/include/std/condition_variable +++ b/libstdc++-v3/include/std/condition_variable @@ -117,7 +117,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const auto __delta = __atime - __c_entry; const auto __s_atime = __s_entry + __delta; - return __wait_until_impl(__lock, __s_atime); + if (__wait_until_impl(__lock, __s_atime) == cv_status::no_timeout) + return cv_status::no_timeout; + // We got a timeout when measured against __clock_t but + // we need to check against the caller-supplied clock + // to tell whether we should return a timeout. + if (_Clock::now() < __atime) + return cv_status::no_timeout; + return cv_status::timeout; } template<typename _Clock, typename _Duration, typename _Predicate> diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc index 0a44c4fa7cf..09a717801e1 100644 --- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc +++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc @@ -51,8 +51,60 @@ void test01() } } +struct slow_clock +{ + using rep = std::chrono::system_clock::rep; + using period = std::chrono::system_clock::period; + using duration = std::chrono::system_clock::duration; + using time_point = std::chrono::time_point<slow_clock, duration>; + static constexpr bool is_steady = false; + + static time_point now() + { + auto real = std::chrono::system_clock::now(); + return time_point{real.time_since_epoch() / 3}; + } +}; + + +void test01_alternate_clock() +{ + try + { + std::condition_variable c1; + std::mutex m; + std::unique_lock<std::mutex> l(m); + auto const expire = slow_clock::now() + std::chrono::seconds(1); + + while (slow_clock::now() < expire) + { + auto const result = c1.wait_until(l, expire); + + // If wait_until returns before the timeout has expired when + // measured against the supplied clock, then wait_until must + // return no_timeout. + if (slow_clock::now() < expire) + VERIFY(result == std::cv_status::no_timeout); + + // If wait_until returns timeout then the timeout must have + // expired. + if (result == std::cv_status::timeout) + VERIFY(slow_clock::now() >= expire); + } + } + catch (const std::system_error& e) + { + VERIFY( false ); + } + catch (...) + { + VERIFY( false ); + } +} + int main() { test01(); + test01_alternate_clock(); return 0; }
diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index c9cd62a..4657af7 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,10 @@ +2018-07-20 Mike Crowe <mac@mcrowe.com> + * include/std/condition_variable (wait_until): Only report timeout + if we really have timed out when measured against the + caller-supplied clock. + * testsuite/30_threads/condition_variable/members/2.cc: Add test + case to confirm above behaviour. + 2018-07-20 Jonathan Wakely <jwakely@redhat.com> PR libstdc++/86595 diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable index 84863a1..a2d146a 100644 --- a/libstdc++-v3/include/std/condition_variable +++ b/libstdc++-v3/include/std/condition_variable @@ -116,7 +116,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const auto __delta = __atime - __c_entry; const auto __s_atime = __s_entry + __delta; - return __wait_until_impl(__lock, __s_atime); + // We might get a timeout when measured against __clock_t but + // we need to check against the caller-supplied clock to tell + // whether we should return a timeout. + if (__wait_until_impl(__lock, __s_atime) == cv_status::timeout) + return _Clock::now() < __atime ? cv_status::no_timeout : cv_status::timeout; + else + return cv_status::no_timeout; } template<typename _Clock, typename _Duration, typename _Predicate> diff --git a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc index 6f9724b..16850a4 100644 --- a/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc +++ b/libstdc++-v3/testsuite/30_threads/condition_variable/members/2.cc @@ -52,8 +52,60 @@ void test01() } } +struct slow_clock +{ + using rep = std::chrono::system_clock::rep; + using period = std::chrono::system_clock::period; + using duration = std::chrono::system_clock::duration; + using time_point = std::chrono::time_point<slow_clock, duration>; + static constexpr bool is_steady = false; + + static time_point now() + { + auto real = std::chrono::system_clock::now(); + return time_point{real.time_since_epoch() / 3}; + } +}; + + +void test01_alternate_clock() +{ + try + { + std::condition_variable c1; + std::mutex m; + std::unique_lock<std::mutex> l(m); + auto const expire = slow_clock::now() + std::chrono::seconds(1); + + while (slow_clock::now() < expire) + { + auto const result = c1.wait_until(l, expire); + + // If wait_until returns before the timeout has expired when + // measured against the supplied clock, then wait_until must + // return no_timeout. + if (slow_clock::now() < expire) + VERIFY(result == std::cv_status::no_timeout); + + // If wait_until returns timeout then the timeout must have + // expired. + if (result == std::cv_status::timeout) + VERIFY(slow_clock::now() >= expire); + } + } + catch (const std::system_error& e) + { + VERIFY( false ); + } + catch (...) + { + VERIFY( false ); + } +} + int main() { test01(); + test01_alternate_clock(); return 0; }