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 448943 - g_timeout_add_seconds() problems
g_timeout_add_seconds() problems
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other Linux
: Normal critical
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2007-06-18 22:50 UTC by Cody Russell
Modified: 2008-04-08 09:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test program (6.23 KB, text/plain)
2007-06-18 22:52 UTC, Cody Russell
  Details
testcase (297 bytes, text/plain)
2008-01-07 04:27 UTC, Matthias Clasen
  Details
Ensure the perturb value is always positive (472 bytes, patch)
2008-03-22 14:29 UTC, Sjoerd Simons
committed Details | Review

Description Cody Russell 2007-06-18 22:50:54 UTC
When using g_timeout_add_seconds(), sometimes the timeout appears to never trigger until there is some sort of other event (e.g., when I press the ctrl or alt key on my keyboard).

When I changed my test program to use g_timeout_add (seconds * 1000, ...) I could not duplicate this behavior.

This is happening in glib trunk.
Comment 1 Cody Russell 2007-06-18 22:52:49 UTC
Created attachment 90243 [details]
Test program

Sorry, I'm recycling my test program from bug 56070 here and I realize it's not a "minimal" test of the bug.  If that's unacceptable let me know and I'll cut it down.

You may have to run this multiple times to reproduce the bug, because it seems to trigger inconsistently for me.
Comment 2 Olivier Crête 2007-11-19 21:58:37 UTC
I can reproduce this too with my Farsight2 unit test, the timeout passed g_main_context_poll() is MAXINT.
Comment 3 Matthias Clasen 2008-01-07 04:27:00 UTC
Created attachment 102297 [details]
testcase

I failed to reproduce this with a simpler test case. 
Can you ?
Comment 4 Cody Russell 2008-01-07 06:55:57 UTC
Sorry, I suddenly can't reproduce it using either your simple test program or the test program from bug #56070 which is where I first noticed this.
Comment 5 Olivier Crête 2008-03-08 04:00:16 UTC
I can reproduce with matthias's testcase after a few tries and glib 2.14.6.
The timeout for the poll() is MAXINT. And more interesting is that the tv_usec of the source's expiration is -71374 (<0). Something is wrong there.

So in g_timeout_prepare, we have sec>0 and msec<-1000 .. and then it does (guint)msec.. which becomes a huge value so it chooses G_MAXINT...

One fix is in  g_timeout_prepare(), to replace "if (msec < 0)", by "while (msec < 0).

Also, in g_timeout_set_expiration(), if the remainder is larger than the perturb, that can cause a negative tv_usec value..
Comment 6 Olivier Crête 2008-03-18 17:09:37 UTC
I'm confirming this bug, and we should fix this asap... It makes the whole api useless
Comment 7 Matthias Clasen 2008-03-18 17:33:24 UTC
Do you have a proposed patch ?
Comment 8 Sjoerd Simons 2008-03-22 14:29:41 UTC
Created attachment 107805 [details] [review]
Ensure the perturb value is always positive

g_timeout_prepare is fine as long as the initially calculated msec is:  -1000 < msec < 1000, which is true as long as the tv_usecs are:  0 < tv_usec < 1000 * 1000..

Now if we look in g_timeout_set_expiration, then this is the case as long as timer_perturb >= 0. Just do the code by hand for timber_perturb = -10, gran = 50 and expiration.tv_usec = 0.. Unfortunately (gint)g_str_hash isn't guaranteed to be >= 0, hence this trivial fix..
Comment 9 Matthias Clasen 2008-04-03 04:54:41 UTC
2008-04-03  Matthias Clasen  <mclasen@redhat.com>

        Bug 448943 – g_timeout_add_seconds() problems

        * glib/gmain.c (g_timeout_set_expiration): Prevent expiration
        time going negative. Reported by Cody Russell, analyzed by
        Olivier Crete, patch by Sjoerd Simons.
Comment 10 Sjoerd Simons 2008-04-04 09:49:43 UTC
Unfortunately the commited patch is slightly different then my patch and wrong...

Committed is:
-        timer_perturb = g_str_hash (session_bus_address);
+        timer_perturb = ABS (g_str_hash (session_bus_address));

while my patch did:
-        timer_perturb = g_str_hash (session_bus_address);
+        timer_perturb = ABS ((gint) g_str_hash (session_bus_address));

Note that the cast to gint before using was removed. As g_str_hash returns a guint the commited patch doesn't actually have any effect at all :(. 

Comment 11 Matthias Clasen 2008-04-04 13:04:18 UTC
I swear I looked and removed the cast because the function already returned a gint...now that I look again, you are of course right, it returns guint. Sorry about that, correcting it now.
Comment 12 Jean-Yves Lefort 2008-04-08 09:40:26 UTC
Note that you're calling g_str_hash() once more than needed since ABS() evaluates the argument twice. I have verified that adding G_GNUC_PURE to g_str_hash() fixes the problem. I suggest to go through the GLib API and add optimization attributes as appropriate.