Created attachment 207099 [details] [review]
gasyncqueue: don't use deprecated g_cond_timed_wait()

Fix it to use g_cond_wait_until() instead.

I don't think we can credibility use such hacks on a non-deprecated API.  We need to deprecate this one and introduce a new one based on either relative or absolute monotonic time (or both).

Review of attachment 207099 [details] [review]:

::: glib/gasyncqueue.c
@@ +418,3 @@
+      if (end_time != NULL)
+	{
+	  wait_time = g_get_monotonic_time () - g_get_real_time ();

I don't really get this...how is there a relationship between the monotonic clock and the wall clock?

(In reply to comment #2)
> I don't think we can credibly use such hacks on a non-deprecated API.

Well, but we already are; All this patch does is basically inline the definition of g_cond_timed_wait() from gthread-deprecated.c rather than calling it.

> We need to deprecate this one and introduce a new one based on either
> relative or absolute monotonic time (or both).

Sure, I suppose that if g_cond_timed_wait() is bad API then it would follow that g_async_queue_timed_pop() is bad too...


(In reply to comment #3)
> I don't really get this...how is there a relationship between the monotonic
> clock and the wall clock?

@end_time is in real time, but g_cond_wait_until() takes a monotonic time. So we need "monotonic_time + (end_time - real_time)". (I guess the way the math is done here is not totally obvious, but that's how it's done in gthread-deprecated.c...)

Created attachment 207255 [details] [review]
gasyncqueue: deprecate GTimeVal-based methods, add monotonic ones

=====

ok, this adds new methods (better names welcome), deprecates the old
ones, etc.

Review of attachment 207255 [details] [review]:

Ya.  This is much better.

about the names: 'timeout' suggests a relative time.  I think following the _until convention may be good here.

pop_until() sounds weird, so maybe try_pop_until()?

We could also add in the relative version which is probably what most people actually want...  try_pop_for()?  try_pop_with_timeout()?

oh names...

Created attachment 207285 [details] [review]
gasyncqueue: deprecate GTimeVal-based methods, add relative-delay ones

=====

Changed to take a length of time rather than an end time. Since Ryan
said the original name suggested a relative time to him, I just kept
that name.

Review of attachment 207285 [details] [review]:

I'm not crazy about the name, but I'm not crazy about any name that I can think of, so this is as good as any.

Everything else is perfect.  Thanks.

Attachment 207285 [details] pushed as dd553a2 - gasyncqueue: deprecate GTimeVal-based methods, add relative-delay ones

That commit seems to have creater a bug, nautilus started hitting those errors for nautilus-dropbox users:

"GLib (gthread-posix.c): Unexpected error from C library during 'Bad argumentt': pthread_cond_timedwait.  Aborting.
Aborted (core dumped)"


Chris Goller (goller) tracked it to this commit on https://bugs.launchpad.net/ubuntu/+source/nautilus-dropbox/+bug/932627/comments/18

"I think I've tracked it down...

http://ftp.gnome.org/pub/gnome/sources/glib/2.31/glib-2.31.18.news

glib did this change
 * g_async_queue_timed_pop has been deprecated in favor of
  the new g_async_queue_timeout_pop, which uses relative
  delays in microseconds instead of a GTimeVal.

and the exact change in g_async_queue_timed_pop

+ if (end_time != NULL)
+ {
+ m_end_time = g_get_monotonic_time () +
+ (end_time->tv_sec * G_USEC_PER_SEC + end_time->tv_usec -
+ g_get_real_time ());
+ }
+ else
+ m_end_time = -1;
+

end_time->tv_sec * G_USEC_PER_SEC + end_time->tv_usec - g_get_real_time () returns a value far less than 0 and it causes
the m_end_time to be less than 0.

Therefore, this sets m_end_time to negative creating an invalid argument to be passed to pthread_cond_wait.

I think just casting end_time->tv_sec to gint64 is all that is needed."

Created attachment 208196 [details] [review]
gasyncqueue: fix a 32bit overflow in g_async_queue_timed_pop

also, add a test for g_async_queue_timed_pop() and
g_async_queue_timeout_pop() to tests/asyncqueue.c

====

It would be nice if someone on a 32bit machine could verify that the
test does crash without the fix, and doesn't crash with the fix. (It
works fine either way on x86_64.)

Review of attachment 208196 [details] [review]:

::: glib/gasyncqueue.c
@@ +603,3 @@
     {
       m_end_time = g_get_monotonic_time () +
+	((gint64)end_time->tv_sec * G_USEC_PER_SEC + end_time->tv_usec -

Looks right.

::: glib/tests/asyncqueue.c
@@ +187,3 @@
+  diff = end - start;
+  g_assert_cmpint (diff, >=, G_USEC_PER_SEC / 10);
+  g_assert_cmpint (diff, <, 1.1 * (G_USEC_PER_SEC / 10));

I don't like having a test case which is going to assert if it happens to run too slowly.  If we were talking human timescales like 30-60 seconds say, that's one thing, but here the failure would happen above 100 milliseconds.  Right?

This kind of thing is tricky because we actually have no documentation about what environment we expect the tests to be run under.  But I could easily imagine if e.g. running them under Linux' idle class schedluing (chrt --idle 0 mytest) you could get that kind of latency.

ok, trying the new test on 2.31.18 before the code patch:

/asyncqueue/timed: GLib (gthread-posix.c): Unexpected error from C library during 'Invalid argument': pthread_cond_timedwait.  Aborting.

then after:

/asyncqueue/timed: OK

that's on a 32 bits ubuntu precise install

after discussion on IRC, pushed the patch with the test changed to
only check that the delay was less than 1 second.

Attachment 208196 [details] pushed as 88182d3 - gasyncqueue: fix a 32bit overflow in g_async_queue_timed_pop