GNOME Bugzilla – Bug 673607
invalid assumption in g_cond_wait_until() / g_get_monotonic_time() API
Last modified: 2014-02-23 04:23:16 UTC
The following invalid assumption is implicit in the API of these two functions, and explicit in g_cond_wait_until() documentation. It appears that these two functions: g_get_monotonic_time() g_cond_wait_until() are intended to be used together. At least this is what the example code for g_cond_wait_until() does, in: http://developer.gnome.org/glib/2.31/glib-Threads.html#g-cond-wait-until However, if we look in detail at what they do, combining them will break systems that don't implement the optional pthread_condattr_setclock() facility. g_get_monotonic_time() always uses CLOCK_MONOTONIC timer whenever it is available, and only falls back to CLOCK_REALTIME when it's not: #ifdef CLOCK_MONOTONIC clock_gettime (CLOCK_MONOTONIC, &ts); #else clock_gettime (CLOCK_REALTIME, &ts); #endif In turn, g_cond_wait_until() just delegates to pthread_cond_timedwait(), as specified in the documentation. Whether g_cond_wait_until() will use CLOCK_MONOTONIC or CLOCK_REALTIME, it's determined by the Gcond constructor: g_cond_impl_new(). In g_cond_impl_new(), we see that pthread_condattr_setclock() is called only when available, to select CLOCK_MONOTONIC timer, and otherwise skipped: #if defined (HAVE_PTHREAD_CONDATTR_SETCLOCK) && defined (CLOCK_MONOTONIC) pthread_condattr_setclock (&attr, CLOCK_MONOTONIC); #endif Since this criteria doesn't match with the one for g_get_monotonic_time(), the result is that on systems that don't implement pthread_condattr_setclock(), the recommended use of g_cond_wait_until() produces broken code (timeout always fails). This was found to be the case when running the libsoup testsuite on Debian GNU/kFreeBSD (see http://bugs.debian.org/663056).
Created attachment 211431 [details] [review] patch Proposed fix.
The other obvious approach here is to try to convert montonic time to wallclock time in the case that the setclock() API is missing. I'm not sure that we should degrade the entire g_get_monotonic_time() API just because the user may call g_cond_wait_until() with the result.
Hi Ryan, I can see where you're going, but it gets very ugly. I suppose you'd want (pseudocode): clock_gettime (CLOCK_MONOTONIC, &cur_monotonic); clock_gettime (CLOCK_REALTIME, &cur_realtime); end_time += cur_realtime - cur_monotonic; pthread_cond_timedwait (&end_time); On one hand this code has a race condition (as it could be preempted between the clock_gettime calls). On the other, it's not correct because in fact g_get_monotonic_time() is already degraded and maybe it already returns realtime: #ifdef CLOCK_MONOTONIC clock_gettime (CLOCK_MONOTONIC, &ts); #else clock_gettime (CLOCK_REALTIME, &ts); #endif So we'd need to ifdef the whole thing, which gets even uglier. TBH, I don't think it's worth it. pthread_condattr_setclock() is an easy call to implement. The only big issue with it happens when programs make wrong assumptions. If glib just failed to build or returned ENOSYS, this would give a reasonable hint that this function is required. So if you're against degrading g_get_monotonic_time(), I suggest you go all the way and make CLOCK_MONOTONIC and pthread_condattr_setclock() an absolute requirement in functions that use it. If they're missing in OS facilities, then any of: - Fail to build. - Disable presence of the whole function. - Return ENOSYS. This will give a much better hint to OS implementors that pthread_condattr_setclock() is needed.
*** Bug 703594 has been marked as a duplicate of this bug. ***
Bug #703594 has a patch for this, that just converts the monotonic time to realtime if a system has monotonic clock but no way to specify that for a condition variable. Of course this is racy, but it's as racy as using the realtime for the condition variables, thus improving the situation as good as possible.
We're now talking about supporting the monotonic clock on Mac OS, where CLOCK_MONOTONIC does not exist -- so this issue is about to get worse. Doing a bit more research, it seems that most systems have a way to reliably implement condition variables based on either monotonic time or relative time (ie: not using wallclock time): Window is non-POSIX (and has a relative time API on its condition variables). GNU/Linux has pthread_condattr_setclock() Android has pthread_cond_timedwait_monotonic{,_np}() Mac OS has pthread_cond_timedwait_relative_np() FreeBSD/OpenBSD/NetBSD have pthread_condattr_setclock() In the relative case we can (safely) calculate the specified time, minus the current monotonic time just before making the call. I think we should try to do this without the fallback to using the wall-clock and see if we get any complaints... One question for Sebastian: what's the story with _timedwait_monotonic_np() vs. _timedwait_monotonic()? Different Android versions?
Seems Android has pthread_cond_timedwait_relative_np() as well, and the non-_np() suffix is deprecated. (presumably they felt bad for using the posix namespace for a non-standard feature, but it was too late to fix it). https://gitorious.org/freebroid/bionic/commit/a2f5e212448f36f0b35cf695d13bb4defdb4472e In light of this ("over 4 years ago") I think we could rip out the timedwait_monotonic{,np}() stuff entirely and just use the relative API if it's available.
Created attachment 269642 [details] [review] Fix g_cond_wait_until() vs. monotonic time We've had a relatively rocky path with g_cond_wait_until() on systems that either don't support pthread_condattr_setclock() or where g_get_monotonic_time() is not based on CLOCK_MONOTONIC (ie: Android and Mac OS). Fortunately, both of these platforms seem to share pthread_cond_timedwait_relative_np() which allows us to implement g_cond_wait_until() without races. With this patch, we now require that one of pthread_condattr_setclock() or pthread_cond_timedwait_relative_np() exists. A quick look around suggests that this is true for all platforms that we care about. This patch removes our use of pthread_cond_timedwait_monotonic() and pthread_cond_timedwait_monotonic_np() which were Android-only APIs.
Created attachment 269643 [details] [review] tests: add a test for g_cond_wait_until()
Sounds like a good plan, yes. And I think your guesses for the reasons of the _np vs. non_np variant are correct, and I'm fine with dropping support for the non_np variant.
Review of attachment 269642 [details] [review]: Looks good to me
Attachment 269642 [details] pushed as 1de36e7 - Fix g_cond_wait_until() vs. monotonic time Attachment 269643 [details] pushed as 436d77f - tests: add a test for g_cond_wait_until()
We're now broken on Hurd as a result of missing pthread_condattr_setclock() support (at runtime). I contacted the Hurd maintainers about this and they plan to fix it on their side.