This is an archive of the discontinued LLVM Phabricator instance.

Always have std::condition_variable wait on CLOCK_MONOTONIC on Android
AbandonedPublic

Authored by tomcherry on Mar 27 2018, 12:04 PM.

Details

Summary

std::condition_variable currently waits using chrono::system_clock
which uses CLOCK_REALTIME in all cases. This behavior is virtually
never intended and has been the source of numerous user visible bugs
on Android, as Android is prone to system clock changes.

Android already has a change that converts pthread condition variable
waits from CLOCK_REALTIME to CLOCK_MONOTONIC to work around this
issue, but that still leaves a race from the point the original timeout
was calculated to when the conversion happens.

This change, paired with an accompanying libc change in Android,
creates a mechanism to always wait on CLOCK_MONOTONIC, which
completely avoids the above race.

Change-Id: I0246d85df106daa7a3e59ed2bb21180c86d6722d

Event Timeline

tomcherry created this revision.Mar 27 2018, 12:04 PM

My first response (based on looking at the patch, and doing a bit of research) is:

  • This is needed
  • I'm not sure this is quite the right approach.

I'll have some more comments after some more looking/research.

Thanks for proposing this!

Thanks for the comments. I'm very open to suggestions if you have any better way of doing this.

This change in its current form is unfortunate, but here're the issues that lead me here:

The only POSIX based solution for waiting on this clock is pthread_condattr_setclock(), but we can't use it for two reasons

  • We need a constexpr constructor and this would no longer be the case if we added a call to pthread_condattr_setclock()
  • Older Android releases that we need to support do not have pthread_condattr_setclock()

We could just rewrite all of std::condition_variable without POSIX pthreads at all, e.g. https://reviews.llvm.org/D36767

  • We probably don't actually want to do this

So that leaves me with making non-POSIX extensions in Android that can solve this.

  • But these need to be backwards compatible
  • Older 32bit Android releases support waiting with CLOCK_MONOTONIC through pthread_cond_timedwait_monotonic_np(), but they don't have a bit that could be set with an initializer to request this clock.
  • Older 64bit Android releases do not have pthread_cond_timedwait_monotonic_np(), though all of its releases do have a bit that can be set in the initializer to request CLOCK_MONOTONIC.

So that's how I got here, including why there's a separate path for 32bit and 64bit.

Tom, large chunks of that last comment should probably be part of the CL description, as it outlines exactly why this approach was chosen. Sure, it's in the phabricator notes, but it would probably be better in a more easily accessible spot (especially since people might have questions about this code in the future).

Update commit message

This revision is now accepted and ready to land.Apr 17 2018, 1:57 PM