Fix WaitableEvent and ConditionVariable::TimedWait to use monotonic time on non-mac posix

Fix WaitableEvent and ConditionVariable::TimedWait to use monotonic time on non-Mac non-NaCl posix

Both use a relative time, and it's important that this relative time is consistent with the wall clock when the system time gets adjusted (e.g. NTP, tlsdate, etc.). The monotonic clock, when available, has that property. Unfortunately, by default, pthread_cond_timedwait takes the system time which doesn't have that property.

On Linux/Chrome OS, we use pthread_condattr_setclock which lets us use the monotonic clock. On Android, we use the non-portable pthread_cond_timedwait_monotonic_np which has the same effect.
Unfortunately, neither is supported by NaCl.
Mac can use the non-portable pthread_cond_timedwait_relative_np which uses a relative time.

BUG=293736
R=thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226378

[piman, 2013-09-25 01:09:42]

This is an attempt at fixing this issue on some platforms. I looked around the
web for a solution on Mac but haven't found one - do you know anything by any
chance? I'll follow up with the NaCl folks to see if we can have a fix there.

[Nico, 2013-10-01 21:20:42]

That's a fun bug.

https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronizati...
File base/synchronization/condition_variable_posix.cc (right):

https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronizati...
base/synchronization/condition_variable_posix.cc:88: &condition_, user_mutex_,
&abstime);
I haven't used it before, but from some googling around it seems that os x has
pthread_cond_timedwait_relative_np():

/* Like pthread_cond_timedwait, but use a relative timeout */
int		pthread_cond_timedwait_relative_np(pthread_cond_t *,
				 pthread_mutex_t *,
				 const struct timespec *);

[Nico, 2013-10-01 21:32:45]

n Tue, Oct 1, 2013 at 2:20 PM, <thakis@chromium.org> wrote:

> That's a fun bug.
>
>
> https://chromiumcodereview.**appspot.com/24158005/diff/**
>
17001/base/synchronization/**condition_variable_posix.cc<https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronization/condition_variable_posix.cc>
> File base/synchronization/**condition_variable_posix.cc (right):
>
> https://chromiumcodereview.**appspot.com/24158005/diff/**
>
17001/base/synchronization/**condition_variable_posix.cc#**newcode88<https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronization/condition_variable_posix.cc#newcode88>
> base/synchronization/**condition_variable_posix.cc:**88: &condition_,
> user_mutex_, &abstime);
> I haven't used it before, but from some googling around it seems that os
> x has pthread_cond_timedwait_**relative_np():
>
> /* Like pthread_cond_timedwait, but use a relative timeout */
> int             pthread_cond_timedwait_**relative_np(pthread_cond_t *,
>                                  pthread_mutex_t *,
>                                  const struct timespec *);
>

But looking through _pthread_cond_wait in pthread_cond.c in Libc from
http://www.opensource.apple.com/release/mac-os-x-1084/ , it seems
that pthread_cond_timedwait() just subtracts gettimeofday() from the
timespec parameter and then does a relative wait based on that, so unless
the system time is changed between the call to gettimeofday in
condition_variable_posix.cc and the call in pthread_cond_timedwait(),
things should be fine on OS X (but eliminating that window is probably
still worth doing).


>
>
https://chromiumcodereview.**appspot.com/24158005/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2013-10-01 21:40:07]

On Tue, Oct 1, 2013 at 2:32 PM, Nico Weber <thakis@chromium.org> wrote:

> n Tue, Oct 1, 2013 at 2:20 PM, <thakis@chromium.org> wrote:
>
>> That's a fun bug.
>>
>>
>> https://chromiumcodereview.**appspot.com/24158005/diff/**
>>
17001/base/synchronization/**condition_variable_posix.cc<https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronization/condition_variable_posix.cc>
>> File base/synchronization/**condition_variable_posix.cc (right):
>>
>> https://chromiumcodereview.**appspot.com/24158005/diff/**
>>
17001/base/synchronization/**condition_variable_posix.cc#**newcode88<https://chromiumcodereview.appspot.com/24158005/diff/17001/base/synchronization/condition_variable_posix.cc#newcode88>
>> base/synchronization/**condition_variable_posix.cc:**88: &condition_,
>> user_mutex_, &abstime);
>> I haven't used it before, but from some googling around it seems that os
>> x has pthread_cond_timedwait_**relative_np():
>>
>> /* Like pthread_cond_timedwait, but use a relative timeout */
>> int             pthread_cond_timedwait_**relative_np(pthread_cond_t *,
>>                                  pthread_mutex_t *,
>>                                  const struct timespec *);
>>
>
> But looking through _pthread_cond_wait in pthread_cond.c in Libc from
> http://www.opensource.apple.com/release/mac-os-x-1084/ , it seems
> that pthread_cond_timedwait() just subtracts gettimeofday() from the
> timespec parameter and then does a relative wait based on that, so unless
> the system time is changed between the call to gettimeofday in
> condition_variable_posix.cc and the call in pthread_cond_timedwait(),
> things should be fine on OS X (but eliminating that window is probably
> still worth doing).
>

Ah ok, I will use that. It can only be better, thanks!


>
>>
>>
https://chromiumcodereview.**appspot.com/24158005/<https://chromiumcodereview...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2013-10-01 21:55:26]

fix mac

[piman, 2013-10-01 22:41:03]

PTAL with the mac fix.

[Nico, 2013-10-01 23:01:53]

lgtm, thanks!

Having a DISABLED_ test for this isn't ideal, but I can't think of anything
better either.

It's probably worth notifying someone one the nacl team about this hole in their
api?

https://chromiumcodereview.appspot.com/24158005/diff/32001/base/synchronizati...
File base/synchronization/condition_variable_posix.cc (right):

https://chromiumcodereview.appspot.com/24158005/diff/32001/base/synchronizati...
base/synchronization/condition_variable_posix.cc:77: #if defined(OS_NACL)
Maybe add a "// See comment in constructor for why this is different in NaCl"
here

[piman, 2013-10-01 23:16:33]

https://chromiumcodereview.appspot.com/24158005/diff/32001/base/synchronizati...
File base/synchronization/condition_variable_posix.cc (right):

https://chromiumcodereview.appspot.com/24158005/diff/32001/base/synchronizati...
base/synchronization/condition_variable_posix.cc:77: #if defined(OS_NACL)
On 2013/10/01 23:01:53, Nico wrote:
> Maybe add a "// See comment in constructor for why this is different in NaCl"
> here

Done.

[commit-bot: I haz the power, 2013-10-01 23:17:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/24158005/43001

[commit-bot: I haz the power, 2013-10-02 00:30:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 01:18:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/24158005/69001

[piman, 2013-10-02 01:25:34]

Committed patchset #9 manually as r226378 (presubmit successful).

[Alexander Potapenko, 2013-10-03 09:16:17]

https://codereview.chromium.org/24158005/diff/69001/base/synchronization/cond...
File base/synchronization/condition_variable_unittest.cc (right):

https://codereview.chromium.org/24158005/diff/69001/base/synchronization/cond...
base/synchronization/condition_variable_unittest.cc:209:
TEST_F(ConditionVariableTest, DISABLED_TimeoutAcrossSetTimeOfDay) {
A disabled test will rot in no time, because nobody is going to run it.
Also, the comment references a bug which is already marked as fixed and which
doesn't contain any details relevant to this test.

[piman, 2013-10-03 18:28:16]

On 2013/10/03 09:16:17, Alexander Potapenko wrote:
>
https://codereview.chromium.org/24158005/diff/69001/base/synchronization/cond...
> File base/synchronization/condition_variable_unittest.cc (right):
> 
>
https://codereview.chromium.org/24158005/diff/69001/base/synchronization/cond...
> base/synchronization/condition_variable_unittest.cc:209:
> TEST_F(ConditionVariableTest, DISABLED_TimeoutAcrossSetTimeOfDay) {
> A disabled test will rot in no time, because nobody is going to run it.
> Also, the comment references a bug which is already marked as fixed and which
> doesn't contain any details relevant to this test.

ideas welcome.