After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 673607 - invalid assumption in g_cond_wait_until() / g_get_monotonic_time() API
invalid assumption in g_cond_wait_until() / g_get_monotonic_time() API
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gthread
2.32.x
Other other
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 703594 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-04-05 20:55 UTC by Robert Millan
Modified: 2014-02-23 04:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (599 bytes, patch)
2012-04-05 20:58 UTC, Robert Millan
none Details | Review
Fix g_cond_wait_until() vs. monotonic time (4.80 KB, patch)
2014-02-19 00:06 UTC, Allison Karlitskaya (desrt)
committed Details | Review
tests: add a test for g_cond_wait_until() (1.80 KB, patch)
2014-02-19 00:06 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Robert Millan 2012-04-05 20:55:47 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).
Comment 1 Robert Millan 2012-04-05 20:58:43 UTC
Created attachment 211431 [details] [review]
patch

Proposed fix.
Comment 2 Allison Karlitskaya (desrt) 2012-04-06 02:38:50 UTC
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.
Comment 3 Robert Millan 2012-04-06 09:54:34 UTC
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.
Comment 4 Dan Winship 2013-07-05 14:50:00 UTC
*** Bug 703594 has been marked as a duplicate of this bug. ***
Comment 5 Sebastian Dröge (slomo) 2013-07-19 09:52:02 UTC
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.
Comment 6 Allison Karlitskaya (desrt) 2014-02-18 23:29:54 UTC
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?
Comment 7 Allison Karlitskaya (desrt) 2014-02-18 23:35:06 UTC
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.
Comment 8 Allison Karlitskaya (desrt) 2014-02-19 00:06:38 UTC
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.
Comment 9 Allison Karlitskaya (desrt) 2014-02-19 00:06:42 UTC
Created attachment 269643 [details] [review]
tests: add a test for g_cond_wait_until()
Comment 10 Sebastian Dröge (slomo) 2014-02-19 19:44:22 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2014-02-19 19:53:14 UTC
Review of attachment 269642 [details] [review]:

Looks good to me
Comment 12 Allison Karlitskaya (desrt) 2014-02-20 23:00:17 UTC
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()
Comment 13 Allison Karlitskaya (desrt) 2014-02-23 04:23:16 UTC
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.