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 648145 - doesn't update time on return from suspend
doesn't update time on return from suspend
Status: RESOLVED FIXED
Product: gnome-screensaver
Classification: Deprecated
Component: dialog
3.0.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-screensaver maintainers
gnome-screensaver maintainers
Depends on:
Blocks:
 
 
Reported: 2011-04-18 19:17 UTC by Bill Nottingham
Modified: 2011-07-25 15:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to fix the clock getting out of sync (5.21 KB, patch)
2011-07-20 20:58 UTC, Alexandre Rostovtsev
none Details | Review
patch to fix the clock getting out of sync, v2 (4.43 KB, patch)
2011-07-23 08:01 UTC, Alexandre Rostovtsev
none Details | Review
patch to fix the clock getting out of sync, v3 (4.40 KB, patch)
2011-07-23 08:07 UTC, Alexandre Rostovtsev
committed Details | Review

Description Bill Nottingham 2011-04-18 19:17:21 UTC
If I suspend to RAM, on resume the clock in gnome-screensaver's unlock dialog isn't updated (it shows the pre-suspend time). Once I unlock, the shell clock is properly updated.

gnome-shell-3.0.0.2-2.fc15.x86_64
gnome-screensaver-3.0.0-1.fc15.x86_64
Comment 1 Alexandre Rostovtsev 2011-07-20 20:58:06 UTC
Created attachment 192336 [details] [review]
patch to fix the clock getting out of sync

Glib implements its g_timeout_* timers using CLOCK_MONOTONIC, which, at least on Linux, completely ignores suspend/resume. Therefore, updating the panel clock using g_timeout_add() once per minute (as gnome-screensaver is currently doing) results in the wrong time being displayed on resume from suspend.

This patch adds a cheap (low impact on power, and no UI updates in the default case) 2-second timer to check that the panel clock has not gotten out of sync with the machine's clock (e.g. when the machine resumes from suspend or hibernate, or if the machine's clock is reset by ntp).

In addition, the patch fixes the watchdog timer to use g_timeout_add_seconds() to save a bit of power.
Comment 2 Ray Strode [halfline] 2011-07-22 14:28:09 UTC
I'm not sure I like the idea of having two timeouts.

I say we either drop the optimization here in the code:

tatic void•
queue_clock_update (GSWindow *window)•
{•
        int timeouttime;•
        struct timeval tv;•
•
        gettimeofday (&tv, NULL);•
        timeouttime = (G_USEC_PER_SEC - tv.tv_usec) / 1000 + 1;•
•
        /* timeout of one minute if we don't care about the seconds */•
        timeouttime += 1000 * (59 - tv.tv_sec % 60);•
•
        window->priv->clock_update_id = g_timeout_add (timeouttime, (GSourceFunc)update_clock_timer, window);•
}•

(so just do g_timeout_add_seconds (1, ...))

or we use timerfd_create(CLOCK_REALTIME,  TFD_TIMER_CANCELON_SET) instead of g_timeout_add .

I think the former is easiest for now, we should potentially think about adding the latter to gdatetime in glib.
Comment 3 Ray Strode [halfline] 2011-07-22 15:06:06 UTC
i filed Bug 655129 to handle the glib rfe
Comment 4 Alexandre Rostovtsev 2011-07-22 15:42:06 UTC
(In reply to comment #2)
> (so just do g_timeout_add_seconds (1, ...))

I don't like the idea of a clock on my computer that might be up to 1 second off. It's not a 15-year old microwave oven with a dodgy rtc chip, it's a modern laptop with ntp. Its clocks are supposed to be *accurate*.

> or we use timerfd_create(CLOCK_REALTIME,  TFD_TIMER_CANCELON_SET) instead of
> g_timeout_add .
> 
> I think the former is easiest for now, we should potentially think about adding
> the latter to gdatetime in glib.

That would be the "proper" solution, and having such an API in glib would be extremely useful, but it would mean users don't see the result until gnome-3.2.


Incidentally: my memory may be failing me, but I don't think the gnome2 clock applet suffered from the skew problem. Unfortunately, I don't have a gnome-2 machine to test.
Comment 5 Ray Strode [halfline] 2011-07-22 23:55:44 UTC
That's fine, the add_seconds wasn't my main point.  My points are:

1) We don't need two timeouts running at the same time.
2) We don't have a way right now to get notifications about skew, but will if/when bug 655129 get fixed
3) the minutes-only case has the bug, but the minutes-seconds case doesn't, since it updates every second anyway.
4) in the interim, before bug 655129 gets fixed, we should just make the minutes-only case wake up as frequently or nearly as frequently as the minutes-seconds case

Interested in cooking a patch without the second timeout?
Comment 6 Alexandre Rostovtsev 2011-07-23 08:01:18 UTC
Created attachment 192523 [details] [review]
patch to fix the clock getting out of sync, v2

OK, here is a patch that uses only one timeout, both to check whether the clock has gotten out of sync (using g_timeout_add_seconds) and to update the clock on predicted minute boundaries (using g_timeout_add). Plus, it should be a tiny bit more efficient than my previous patch since it's allocating GDateTime objects once per minute instead of once every 2 seconds.
Comment 7 Alexandre Rostovtsev 2011-07-23 08:07:48 UTC
Created attachment 192524 [details] [review]
patch to fix the clock getting out of sync, v3

Same as above, but without an unused variable (somehow missed it until the patch was already uploaded).