[1/2] condition_variable: Report early wakeup of wait_until as no_timeout

Message ID 20180710100929.32704-1-mac@mcrowe.com
State New
Headers show
Series
  • [1/2] condition_variable: Report early wakeup of wait_until as no_timeout
Related show

Commit Message

Mike Crowe July 10, 2018, 10:09 a.m.
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
---
 libstdc++-v3/ChangeLog                      | 5 +++++
 libstdc++-v3/include/std/condition_variable | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

--
2.11.0

BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.

Comments

Jonathan Wakely July 20, 2018, 10:53 a.m. | #1
On 10/07/18 11:09 +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.

The patch looks good, thanks.

Can we come up with a test for this, using a user-defined clock that
jumps back?


>[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=41861
>---
> libstdc++-v3/ChangeLog                      | 5 +++++
> libstdc++-v3/include/std/condition_variable | 8 +++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>index cceef0271ae..ea7875ace9f 100644
>--- a/libstdc++-v3/ChangeLog
>+++ b/libstdc++-v3/ChangeLog
>@@ -1,3 +1,8 @@
>+2018-07-09  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.
>+
> 2018-07-06  Fran├žois Dumont  <fdumont@gcc.gnu.org>
>
>        * include/debug/functions.h (__gnu_debug::__check_string): Move...
>diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
>index 84863a162d6..a2d146a9b09 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>
>--
>2.11.0
>
>BrightSign considers your privacy to be very important. The emails you send to us will be protected and secured. Furthermore, we will only use your email and contact information for the reasons you sent them to us and for tracking how effectively we respond to your requests.
Mike Crowe July 20, 2018, 11:30 a.m. | #2
On Friday 20 July 2018 at 11:53:38 +0100, Jonathan Wakely wrote:
> On 10/07/18 11:09 +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.
> 
> The patch looks good, thanks.
> 
> Can we come up with a test for this, using a user-defined clock that
> jumps back?

That's a good idea. I'll do that.

Thanks.

Mike.
Jonathan Wakely July 20, 2018, 11:44 a.m. | #3
On 20/07/18 12:30 +0100, Mike Crowe wrote:
>On Friday 20 July 2018 at 11:53:38 +0100, Jonathan Wakely wrote:
>> On 10/07/18 11:09 +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.
>>
>> The patch looks good, thanks.
>>
>> Can we come up with a test for this, using a user-defined clock that
>> jumps back?
>
>That's a good idea. I'll do that.

Thanks.

There are "broken" clocks in testsuite/30_threads/this_thread/60421.cc
and testsuite/30_threads/timed_mutex/try_lock_until/57641.cc but I'm
not sure if either of them is reusable. A custom one for this test
should be reasonably easy.

Patch

diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
index cceef0271ae..ea7875ace9f 100644
--- a/libstdc++-v3/ChangeLog
+++ b/libstdc++-v3/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-07-09  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.
+
 2018-07-06  Fran├žois Dumont  <fdumont@gcc.gnu.org>

        * include/debug/functions.h (__gnu_debug::__check_string): Move...
diff --git a/libstdc++-v3/include/std/condition_variable b/libstdc++-v3/include/std/condition_variable
index 84863a162d6..a2d146a9b09 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>