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 655129 - GDateTime could provide api for implementing wall clocks
GDateTime could provide api for implementing wall clocks
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 656627
 
 
Reported: 2011-07-22 15:05 UTC by Ray Strode [halfline]
Modified: 2014-05-01 22:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gmain: Clarify that timeouts are in terms of monotonic time (3.76 KB, patch)
2011-08-13 15:24 UTC, Colin Walters
none Details | Review
gmain: Add g_timeout_real_time_source_new() (4.90 KB, patch)
2011-08-13 15:24 UTC, Colin Walters
reviewed Details | Review
Add Linux timerfd_create() backend for g_timeout_real_time_source_new() (4.62 KB, patch)
2011-08-13 15:24 UTC, Colin Walters
none Details | Review
gmain: Clarify that timeouts are in terms of monotonic time (3.76 KB, patch)
2011-08-16 08:34 UTC, Colin Walters
committed Details | Review
gmain: Add g_timeout_real_time_source_new() (4.71 KB, patch)
2011-08-16 08:34 UTC, Colin Walters
none Details | Review
Add Linux timerfd_create() backend for g_timeout_real_time_source_new() (5.45 KB, patch)
2011-08-16 08:34 UTC, Colin Walters
none Details | Review
g_timeout_real_time_add_full: New convenience wrapper (2.77 KB, patch)
2011-08-16 08:34 UTC, Colin Walters
none Details | Review
Add g_date_time_create_watch() (9.00 KB, patch)
2011-08-18 08:56 UTC, Colin Walters
none Details | Review
Add Linux timerfd backend (5.26 KB, patch)
2011-08-18 08:57 UTC, Colin Walters
none Details | Review
GTimeZoneMonitor patch (1.46 KB, patch)
2011-08-18 08:57 UTC, Colin Walters
none Details | Review
gdatetime: Add g_date_time_create_watch() (11.49 KB, patch)
2011-08-22 12:52 UTC, Colin Walters
committed Details | Review
Add Linux timerfd_create() backend for g_date_time_create_watch() (5.27 KB, patch)
2011-08-22 12:52 UTC, Colin Walters
committed Details | Review
GTimeZoneMonitor: Revert addition of this class (18.95 KB, patch)
2011-08-22 12:53 UTC, Colin Walters
committed Details | Review
proposed alternate API (28.34 KB, patch)
2011-08-30 13:26 UTC, Allison Karlitskaya (desrt)
reviewed Details | Review
g_get_monotonic_time: fix race condition (1.72 KB, patch)
2011-08-30 13:51 UTC, Dan Winship
accepted-commit_now Details | Review
g_get_monotonic_time: fix race condition (1.75 KB, patch)
2011-08-30 20:43 UTC, Dan Winship
committed Details | Review
g_date_time_source_new: Fix a race condition (6.80 KB, patch)
2011-08-30 21:17 UTC, Colin Walters
none Details | Review
Revert "gdatetime: Add g_date_time_source_new()" (14.46 KB, patch)
2011-08-31 04:40 UTC, Allison Karlitskaya (desrt)
committed Details | Review
New GTimeout.add_real() API (23.10 KB, patch)
2011-08-31 04:41 UTC, Allison Karlitskaya (desrt)
none Details | Review

Description Ray Strode [halfline] 2011-07-22 15:05:37 UTC
Apps occasionally need to show a wall clock (say the shell, gdm, gnome-screensaver, the fallback panel) etc.

The traditional way to implement the clock is to do something like
g_timeout_add (some interval, update_clock, ...);

where update_clock queries the wall clock time and updates a label somewhere or so.

This means the clock will potentially be "wrong" any time there is clock skew (say following a resume from suspend, or following an ntp update) until the next time the timeout fires.

To mitigate this, the timeout interval needs to be made short, say once a second.  That's fine and required if the clock is displaying seconds, but fairly wasteful if its only showing with minute granularity.

The kernel has a way to notify applications of clock skew when using timerfds.  
Basically, the application creates a timer with the TFD_TIMER_CANCELON_SET flag for the requested real-time interval, and if skew happens before that interval expires then the fd fires early.

See http://article.gmane.org/gmane.linux.kernel/1132138

It would be good if g_date_time had a g_date_time_timeout_add_seconds (or whatever) call which woke up every time the wall clock should get updated    Internally it could use timerfd_create(CLOCK_REALTIME, TFD_TIMER_CANCELON_SET) for platforms that support it, or g_timeout_add_seconds(1, ...) for platforms that don't.

I guess it could offer a full blown GDateTimeSource
Comment 1 Colin Walters 2011-08-13 15:24:41 UTC
Created attachment 193772 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

Also note that monotonic time does not include time spent while
suspended (at least on Linux).
Comment 2 Colin Walters 2011-08-13 15:24:45 UTC
Created attachment 193773 [details] [review]
gmain: Add g_timeout_real_time_source_new()

Several different codebases in GNOME want to implement wall clocks.
While we could pretty easily share a private library, it's not a
substantial amount of code, and GLib already has a lot of the
necessary system-specific detection and handling infrastructure.

Note this initial implementation just wakes up once a second; we'll
add the Linux-specific handling in a subsequent commit.
Comment 3 Colin Walters 2011-08-13 15:24:48 UTC
Created attachment 193774 [details] [review]
Add Linux timerfd_create() backend for g_timeout_real_time_source_new()

This makes the source efficient on Linux.

Untested!  TFD_TIMER_CANCEL_ON_SET was only merged after 2.6.28.
Comment 4 Matthias Clasen 2011-08-13 21:08:03 UTC
Review of attachment 193773 [details] [review]:

::: glib/gmain.c
@@ +4190,3 @@
+ * @function: function to call
+ * @data:     data to pass to @function
+ * @notify:   function to call when the timeout is removed, or %NULL

A rare case: too much docs... the function only has one parameter.

@@ +4198,3 @@
+ *
+ * Note that the timeout will fire as soon as the specified wall clock
+ * time has expired, or the system clock is reset in any way (forwards

Maybe 'changed' is clearer than 'reset' here ?

@@ +4203,3 @@
+ * For example, this function allows implementing a HOUR:MINUTE clock
+ * display in an efficient way.  In this case, you would want to
+ * specify an @interval of 60.

Might be good to turn this into an actual example - that would also show that you have to get the new time to display in the callback, instead of relying on the interval.

::: glib/tests/timeout.c
@@ +103,3 @@
+ * we would have some way to test the behavior when the system clock
+ * changes, but that's hard to do without requiring root privileges
+ * and using system-specific interfaces.

Won't this test also fail in the absence of timerfd support ?
Because then you'll fall back to waking up every second...

@@ +124,3 @@
+
+  realtime_cur = g_get_real_time ();
+

I think you also want to g_assert (fired) ?
Comment 5 Colin Walters 2011-08-16 08:34:02 UTC
Created attachment 193911 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

No changes, reattaching for ordering
Comment 6 Colin Walters 2011-08-16 08:34:27 UTC
Created attachment 193912 [details] [review]
gmain: Add g_timeout_real_time_source_new()

Update docs and test for matthias' suggestions
Comment 7 Colin Walters 2011-08-16 08:34:50 UTC
Created attachment 193913 [details] [review]
Add Linux timerfd_create() backend for g_timeout_real_time_source_new()

Bugs fixed - tested and working now.
Comment 8 Colin Walters 2011-08-16 08:34:56 UTC
Created attachment 193914 [details] [review]
g_timeout_real_time_add_full: New convenience wrapper

Originally I wasn't planning to offer the full suite of wrappers for
_source_new().  However, since it's trivial to do this one, and it turns
out that since GSource isn't presently introspecteable (thus requiring
a C wrapper in gnome-shell), let's just do it here.
Comment 9 Colin Walters 2011-08-16 10:01:11 UTC
Actually while porting gnome-screensaver and gnome-shell I realized this interface is suboptimal for a few reasons:

1) We should make it explicitly a one-shot callback, since the point of the interface is you may be woken up early, so each time you need to recompute the timeout
2) There's a race condition in the timerfd code where if your app does:

  * Update display
  * Rearm timeout

If the clock is set backwards in between the two, then since the function re-gets the current wall clock time and queues an offset based on that, we'll be waiting a long time to update it.  I think the function needs to take the displayed wall clock time as a parameter.


Actually I'm thinking now I'll go with Ray's original suggestion and make g_date_time_create_watch().
Comment 10 Dan Winship 2011-08-16 13:34:19 UTC
(In reply to comment #9)
> 1) We should make it explicitly a one-shot callback, since the point of the
> interface is you may be woken up early, so each time you need to recompute the
> timeout

If we're trying to reduce duplicated code, then don't make the caller recompute it. The source should do it itself. Just tell it how often you want to be woken up (every second, every minute, every hour, etc), and it should take care of everything for you.
Comment 11 Ray Strode [halfline] 2011-08-16 14:45:47 UTC
(In reply to comment #9)
> Actually while porting gnome-screensaver and gnome-shell I realized this
> interface is suboptimal for a few reasons:
> 
> 1) We should make it explicitly a one-shot callback, since the point of the
> interface is you may be woken up early, so each time you need to recompute the
> timeout
> 2) There's a race condition in the timerfd code where if your app does:
> 
>   * Update display
>   * Rearm timeout
> 
> If the clock is set backwards in between the two, then since the function
> re-gets the current wall clock time and queues an offset based on that, we'll
> be waiting a long time to update it.
Oh, I just mentioned this in bug 656627. Guess i should have come here first.

> Actually I'm thinking now I'll go with Ray's original suggestion and make
> g_date_time_create_watch().
If you go with that approach then it could monitor timezone changes, under the hood, too.
Comment 12 Dan Winship 2011-08-16 16:00:49 UTC
(In reply to comment #10)
> If we're trying to reduce duplicated code, then don't make the caller recompute
> it. The source should do it itself.

Or maybe better, have a g_real_timeout_add() that does realtime timeouts, and then the clock-specific source could use g_source_add_child_source() to attach one of those to itself.
Comment 13 Colin Walters 2011-08-17 07:33:37 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > 1) We should make it explicitly a one-shot callback, since the point of the
> > interface is you may be woken up early, so each time you need to recompute the
> > timeout
> 
> If we're trying to reduce duplicated code, then don't make the caller recompute
> it. The source should do it itself. Just tell it how often you want to be woken
> up (every second, every minute, every hour, etc), and it should take care of
> everything for you.

Well, there is also the "alarm" case that Evolution has.  So we need both one-shot and periodic.

I think it'll work out OK if we make the clock be an alarm re-set for every minute.
Comment 14 Matthias Clasen 2011-08-17 17:17:03 UTC
Review of attachment 193911 [details] [review]:

Looks good
Comment 15 Colin Walters 2011-08-18 08:56:46 UTC
Created attachment 194108 [details] [review]
Add g_date_time_create_watch()
Comment 16 Colin Walters 2011-08-18 08:57:15 UTC
Created attachment 194109 [details] [review]
Add Linux timerfd backend
Comment 17 Colin Walters 2011-08-18 08:57:51 UTC
Created attachment 194110 [details] [review]
GTimeZoneMonitor patch
Comment 18 Colin Walters 2011-08-18 09:05:01 UTC
This is looking better; note I now expose cancel_on_set as well as making the source explicitly one shot.  The GDateTime is now the implicit target time; you don't need to calculate an offset.

Note though to really fulfill the API contract, I need to detect time zone changes in the cancel_on_set case.  Right now GDateTime lives in glib/, but GTimeZoneMonitor is in gio/ because that's where the file monitoring bits are =(

GTimeZoneMonitor is a really lame class honestly.  I think we should figure out some change where new GDateTime instances pick up the new timezone automatically, at least for Linux.  We could write some duplicate inotify code for glib/.  

A much more elaborate change would be to try pulling the file monitoring code down into glib/, perhaps as an underscore-prefixed _g_private_file_monitor or something.
Comment 19 Matthias Clasen 2011-08-18 16:58:18 UTC
Review of attachment 194108 [details] [review]:

::: glib/gdatetime.c
@@ +2646,3 @@
+ * g_date_time_create_watch:
+ * @datetime: Time to await
+ * @cancel_on_set: Also invoke callback if the system clock changes discontiguously

cancel_on_set seems like a fairly nondescriptive name for this parameter. In particular the 'cancel' part. Maybe something like trigger_on_clock_set instead ?
Comment 20 Matthias Clasen 2011-08-18 17:03:39 UTC
Review of attachment 194108 [details] [review]:

I would still like to see a real example in the docs.
Comment 21 Colin Walters 2011-08-22 11:17:24 UTC
Comment on attachment 193911 [details] [review]
gmain: Clarify that timeouts are in terms of monotonic time

Attachment 193911 [details] pushed as f0db0d2 - gmain: Clarify that timeouts are in terms of monotonic time
Comment 22 Colin Walters 2011-08-22 12:52:29 UTC
Created attachment 194357 [details] [review]
gdatetime: Add g_date_time_create_watch()

* Add example
* Update gtk-doc metadata
* Update public symbols lists
Comment 23 Colin Walters 2011-08-22 12:52:52 UTC
Created attachment 194358 [details] [review]
Add Linux timerfd_create() backend for g_date_time_create_watch()

No real changes; reattaching for ordering
Comment 24 Colin Walters 2011-08-22 12:53:05 UTC
Created attachment 194359 [details] [review]
GTimeZoneMonitor: Revert addition of this class

The main rationale for adding it was to avoid having gnome-shell
mmap'ing /etc/localtime once a second.  However, we can just as easily
run inotify there, and given no one else was clamoring for a way to
detect when the time zone changes, I don't see a need for public API
here - at least not yet.

In the bigger picture, I just don't believe that the vast majority of
applications are going to go out of their way to instantiate and keep
around a random GTimeZoneMonitor class.  And if they do, it's has the
side effect that for other bits of code in the process, local GDateTime
instances may start varying again!

So, if code can't rely on local GDateTime instances being in a
consistent state anyways, let's just do that always.  The
documentation now says that this is the case.  Applications have
always been able to work in a consistent local time zone by
instantiating a zone and then using it for GDateTime constructors.

We fix the "gnome-shell stats /etc/localtime once a second" issue by
using timerfd (in glib) and inotify (in gnome-shell).
Comment 25 Colin Walters 2011-08-22 12:55:09 UTC
(In reply to comment #19)
> Review of attachment 194108 [details] [review]:
> 
> ::: glib/gdatetime.c
> @@ +2646,3 @@
> + * g_date_time_create_watch:
> + * @datetime: Time to await
> + * @cancel_on_set: Also invoke callback if the system clock changes
> discontiguously
> 
> cancel_on_set seems like a fairly nondescriptive name for this parameter. In
> particular the 'cancel' part. Maybe something like trigger_on_clock_set instead

I chose the name for consistency with the Linux timerfd_settime() flag.  Thought about changing it, but I like the consistency and the docs make pretty clear what it does.
Comment 26 Colin Walters 2011-08-22 12:55:26 UTC
(In reply to comment #20)
> Review of attachment 194108 [details] [review]:
> 
> I would still like to see a real example in the docs.

Done.
Comment 27 Allison Karlitskaya (desrt) 2011-08-22 13:43:25 UTC
GTimeZoneMonitor was only ever added as a kludge because gnome-shell wanted such a thing.  Since it's not even used there at all, I have zero attachment to it.

Rip it out :)
Comment 28 Allison Karlitskaya (desrt) 2011-08-22 13:54:01 UTC
about the API addition, I really have just two comments:

first, the API should be presented as a new GMainContext source in the usual way -- not as part of GDateTime's API.

second, we should really add the timezone-change facilities here so that you have one-stop shopping for detecting any changes in the local time.  Colin suggested that we could use inotify on Linux and fallback to one-per-second polling on other OSes (since we already do that for lack of timerfd).  That makes me cringe, but I'd consider it to be acceptable.
Comment 29 Dan Winship 2011-08-22 14:15:39 UTC
(In reply to comment #28)
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.

...

> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux

if we aren't making it part of the GDateTime API, then couldn't we just put the whole thing in gio, and then use GFileMonitor?
Comment 30 Colin Walters 2011-08-22 14:21:20 UTC
(In reply to comment #29)
> 
> if we aren't making it part of the GDateTime API, then couldn't we just put the
> whole thing in gio, and then use GFileMonitor?

Where would it go in gio?  A new GWallClock class?  Kind of ugh =(

I don't have a really strong opinion on whether it's 

g_date_time_create_watch() in gdatetime.h (modeling on g_io_create_watch())
or
g_date_time_source_new() in gmain.h (modeling on g_timeout_source_new())
Comment 31 Colin Walters 2011-08-22 15:18:36 UTC
Comment on attachment 194359 [details] [review]
GTimeZoneMonitor: Revert addition of this class

Attachment 194359 [details] pushed as 5b68b49 - GTimeZoneMonitor: Revert addition of this class
Comment 32 Colin Walters 2011-08-22 15:39:18 UTC
(In reply to comment #28)
> about the API addition, I really have just two comments:
> 
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.

I modeled it after g_io_create_watch().  Do you think it's a bad example?

Side note: the whole "watch" versus "source" terminology is super confusing; I ended up deciding that "source" is right, and only used _watch in the symbol name for consistency with g_io_create_watch().

Maybe we can call it g_date_time_source_new() and keep it in gdatetime.h?

> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux and fallback to one-per-second
> polling on other OSes (since we already do that for lack of timerfd).  That
> makes me cringe, but I'd consider it to be acceptable.

Yeah.
Comment 33 Matthias Clasen 2011-08-22 22:25:50 UTC
(In reply to comment #28)
> about the API addition, I really have just two comments:
> 
> first, the API should be presented as a new GMainContext source in the usual
> way -- not as part of GDateTime's API.
> 
> second, we should really add the timezone-change facilities here so that you
> have one-stop shopping for detecting any changes in the local time.  Colin
> suggested that we could use inotify on Linux and fallback to one-per-second
> polling on other OSes (since we already do that for lack of timerfd).  That
> makes me cringe, but I'd consider it to be acceptable.

Sounds acceptable to me too, in the short term. When glib collapses into a single shared library, we can make it use a file monitor...
Comment 34 Matthias Clasen 2011-08-29 14:28:46 UTC
irc discussion about the name question seems to point towards

g_date_time_create_source
g_date_time_add_watch
g_date_time_add_watch_full
Comment 35 Colin Walters 2011-08-29 14:39:38 UTC
(In reply to comment #34)
> irc discussion about the name question seems to point towards
> 
> g_date_time_create_source

I went with this.

> g_date_time_add_watch
> g_date_time_add_watch_full

We can add these later if someone cares - but the users for this are pretty limited (wall clock implementors and evolution basically).
Comment 36 Allison Karlitskaya (desrt) 2011-08-30 13:20:45 UTC
There is a race here:

Because the user provides the time for themselves and because we are not monitoring for time discontinuities yet, there could be a discontinuity that exists between the time the user acquires the time and the time that the timerfd is actually setup.


Say it's 5:00 by the system time.  We display the time as 5:00, then ask to be woken at 5:01.  While we were doing that, something set the system time back to 4:00.  We've still asked to be woken at 5:01, so it will be 61 minutes before we get called.
Comment 37 Allison Karlitskaya (desrt) 2011-08-30 13:26:16 UTC
Created attachment 195195 [details] [review]
proposed alternate API

we can solve the problem i mention in the previous comment only with a real periodic clock API

any "wake me up at this time" API will be subject to the race...
Comment 38 Colin Walters 2011-08-30 13:43:53 UTC
(In reply to comment #36)
> There is a race here:
> 
> Because the user provides the time for themselves and because we are not
> monitoring for time discontinuities yet, there could be a discontinuity that
> exists between the time the user acquires the time and the time that the
> timerfd is actually setup.

Hm, true.

CPU1:                                                              CPU2:
clock_gettime(CLOCK_REALTIME)
timerfd_create()
                                                                         settimeofday()
timerfd_settime(TFD_CANCEL_ON_SET)


My initial reaction is that we need to take the current time as an argument.  And if we're doing that, we might as well explicitly separate the clock vs alarm cases as you were arguing.

So a new API proposal might be:

GSource *               g_date_time_clock_source_new                    (GDateTime      *now,
                                                                                                       GDateTime      *expiry);

And the example clock:

static gboolean
redisplay_clock (gpointer data)
{
  GSource *source;
  GDateTime *now, *expiry;

  now = g_date_time_new_now_local ();

  g_print ("%02d:%02d\n",
	   g_date_time_get_hour (now),
	   g_date_time_get_minute (now));

  expiry = g_date_time_add_seconds (now, 60 - g_date_time_get_second (now));
  source = g_date_time_clock_source_new (now, expiry);
  g_source_set_callback (source, redisplay_clock, NULL, NULL);
  g_source_attach (source, NULL);
  g_source_unref (source);

  g_date_time_unref (expiry);
  g_date_time_unref (now);

  return FALSE;
}
Comment 39 Dan Winship 2011-08-30 13:51:55 UTC
Created attachment 195202 [details] [review]
g_get_monotonic_time: fix race condition

(noticed while skimming through the code)
Comment 40 Matthias Clasen 2011-08-30 14:02:01 UTC
Review of attachment 195195 [details] [review]:

::: glib/gmain.c
@@ +5042,3 @@
+ * Among them: progression of monotonic time, detected time
+ * discontinuities, detected timezone change or the passage of 1 second
+ * on non-Linux platforms (ie: we have to check manually every second).

I think the 'or the passage of 1 second...' part is obscuring things a bit.

You should separate the description of what events we _want_ to wake up for
from the description of how often platform inadaequacies force us to wake up.

@@ +5047,3 @@
+ * 'ready'.  That allows us the possibility of using child sources for
+ * some events (although we do not currently do so).  Of course, this
+ * means that we'll get a lot of spurious dispatches.

This should maybe say 'we may get some spurious dispatches' - after all, the goal of all of this is to wake up as rarely as possible, right ?

@@ +5291,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

allow-none ?

@@ +5304,3 @@
+ * specified granularity are ignored, but all other changes result in
+ * @function being called.  The call occurs as close after the "top" of
+ * the interval as possible.

But we are not limited to the predefined timespans here, right ? eg it would work fine to use 30*G_TIME_SPAN_SECOND to update a clock every half minute ?
Might be good to clarify that

@@ +5326,3 @@
+ * changes in the time zone or due to a laptop being suspended and
+ * resumed.  In these cases, the function will be called as soon as
+ * possible.

'not at the top' might also happen due to long-running callbacks of higher priority, I guess.

@@ +5332,3 @@
+ * considered to have been created.  This is useful for avoiding race
+ * conditions that might be associated with obtaining a second copy of
+ * the current time for yourself.

I think @now needs some more explanation - what race is there ? the only danger is that you might miss the first tick ?

@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

allow-none?

@@ +5369,3 @@
+ *                              source was created
+ * @function: the #GRealTimeoutFunc to call
+ * @user_data: a pointer to pass to @function

or NULL

::: glib/gmain.h
@@ +159,3 @@
+typedef gboolean (*GRealTimeoutFunc) (GDateTime *datetime,
+                                      gpointer   data);
+

This seems problematic to me - all other sources we have use GSourceFuncs, so you are going to have to do icky casting, and if I just create such a source and use a source func as callback, its not going to work as expected.

::: glib/tests/glib-clock.c
@@ +13,1 @@
+  return TRUE;

The api is clearly designed to make this demo look good :-)
Comment 41 Matthias Clasen 2011-08-30 14:08:25 UTC
Review of attachment 195202 [details] [review]:

::: glib/gmain.c
@@ +2056,3 @@
+	  g_once_init_leave (&clockid, (gsize)CLOCK_MONOTONIC);
+	else
+	  g_once_init_leave (&clockid, (gsize)CLOCK_REALTIME);

I'd prefer to have a single leave call to match the enter call. Use a temporary ?
Comment 42 Colin Walters 2011-08-30 14:08:37 UTC
Review of attachment 195195 [details] [review]:

So...this moves the API around quite a bit.  Before we go farther, what do you think about the approach of tweaking the current API (make it take the current time as a parameter, be clock-only, drop @cancel_on_set)?

::: glib/tests/glib-clock.c
@@ +9,3 @@
+           g_date_time_get_minute (now),
+           g_date_time_get_second (now),
+           g_date_time_get_microsecond (now));

Every 5 seconds?  It'd be clearer if we preserved the clock only updating once a minute.

Or if you want to be a bit fancier, take a --seconds argument or something.
Comment 43 Allison Karlitskaya (desrt) 2011-08-30 14:08:45 UTC
Review of attachment 195195 [details] [review]:

Will adjust comments/docs as well.

::: glib/gmain.c
@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

don't out parameters always implicitly allow NULL?  i'd actually expect that 'allow-none' means that the *returned* value could possibly be NULL.

::: glib/gmain.h
@@ +159,3 @@
+typedef gboolean (*GRealTimeoutFunc) (GDateTime *datetime,
+                                      gpointer   data);
+

We have GCancellableSourceFunc, GSocketSourceFunc, GChildWatchFunc, among others.

::: glib/tests/glib-clock.c
@@ +13,1 @@
+  return TRUE;

I consider this demo to be an extremely typical clock.  We should try to make the typical case easy. :)
Comment 44 Matthias Clasen 2011-08-30 14:25:10 UTC
Review of attachment 195195 [details] [review]:

You should probably split the g_date_time_source_new-removal off into a separate patch.
Comment 45 Matthias Clasen 2011-08-30 14:27:00 UTC
Review of attachment 195202 [details] [review]:

With that change, it looks fine to commit.
Comment 46 Dan Winship 2011-08-30 20:43:41 UTC
Created attachment 195248 [details] [review]
g_get_monotonic_time: fix race condition

updated for Matthias's suggestion
Comment 47 Colin Walters 2011-08-30 21:17:55 UTC
Created attachment 195252 [details] [review]
g_date_time_source_new: Fix a race condition

Ryan Lortie pointed out that we weren't handling the case where the
clock changed between getting the current time and creating a timerfd.
Change the API to accept the current time too.
Comment 48 Colin Walters 2011-08-30 21:19:30 UTC
I didn't actually attempt to reproduce the race condition, but the above is the relatively obvious incremental patch on top of the existing API.

Notable points versus Ryan's:

1) We still attempt to handle the calendaring/event case
2) It's a pretty incremental change and mostly inherits the testing I already did
Comment 49 Matthias Clasen 2011-08-30 22:52:23 UTC
Review of attachment 195195 [details] [review]:

::: glib/gmain.c
@@ +5367,3 @@
+ *             values thereof)
+ * @now: (transfer full) (out): returns the #GDateTime at which the
+ *                              source was created

In most places, we deal with NULL to mean 'not interested in this', but we typically document that fact.
Comment 50 Colin Walters 2011-08-30 23:01:15 UTC
So after more IRC discussion, options:

1) Apply my patch on top of what we have now
2) Rip it out and put it in libgnome-desktop
3) Apply desrt's patch

I've listed this in my personal order of preference roughly.  I'm fine with mclasen picking one of these options.

If 3) is chosen, I'd like to see it be two separate patches as mentioned earlier (revert, then add new API)
Comment 51 Matthias Clasen 2011-08-30 23:58:02 UTC
Review of attachment 195248 [details] [review]:

thanks, looks good now
Comment 52 Dan Winship 2011-08-31 01:16:18 UTC
Comment on attachment 195248 [details] [review]
g_get_monotonic_time: fix race condition

Attachment 195248 [details] pushed as 2955981 - g_get_monotonic_time: fix race condition
Comment 53 Allison Karlitskaya (desrt) 2011-08-31 04:40:41 UTC
Created attachment 195268 [details] [review]
Revert "gdatetime: Add g_date_time_source_new()"

This reverts three commits:

 - 1feb752996b404965a2f58b29a569a273d4374fa
 - 5763c631473539746646697e6a775f6eacaa08e2
 - 21a538934091e1449e0479daf066fa20df2dc2ef
Comment 54 Allison Karlitskaya (desrt) 2011-08-31 04:41:04 UTC
Created attachment 195269 [details] [review]
New GTimeout.add_real() API

Introduce an API to simplify the needs of applications wanting to
periodically update a clock being displayed to the user (and other
similar needs).
Comment 55 Allison Karlitskaya (desrt) 2011-08-31 04:44:06 UTC
Here's the patch that we will need for both choices 2 and 3 and a work-in-progress patch if we decide to go with #3.

Compared to the last iteration this patch introduces support for detecting changes in the timezone.  There are some TODOs still, and I haven't finished incorporating suggestions from the previous review.
Comment 56 Colin Walters 2011-08-31 14:17:20 UTC
(In reply to comment #54)
> Created an attachment (id=195269) [details] [review]
> New GTimeout.add_real() API
> 
> Introduce an API to simplify the needs of applications wanting to
> periodically update a clock being displayed to the user (and other
> similar needs).

Are there any similar needs that aren't actually wall clock displays?
Comment 57 Allison Karlitskaya (desrt) 2011-08-31 14:30:34 UTC
I can't think of any off the top of my head that couldn't be served equally well (or better) with a monotonic timer.
Comment 58 Allison Karlitskaya (desrt) 2011-08-31 17:13:39 UTC
Comment on attachment 195268 [details] [review]
Revert "gdatetime: Add g_date_time_source_new()"

After consulting with Matthias, I've backed out the new API addition while still keeping GTimeZoneMonitor dead.  The new API will also not be added.

We can revisit this at the start of next cycle.
Comment 59 Colin Walters 2011-08-31 19:18:57 UTC
(In reply to comment #57)
> I can't think of any off the top of my head that couldn't be served equally
> well (or better) with a monotonic timer.

Then the whole g_real_time stuff seems like a bad name - if it's for wall clocks, make it g_wall_clock_source_new ()?

(In reply to comment #58)
> (From update of attachment 195268 [details] [review])
> After consulting with Matthias, I've backed out the new API addition while
> still keeping GTimeZoneMonitor dead.  The new API will also not be added.
> 
> We can revisit this at the start of next cycle.

Sounds fine to me.  In the meantime I have to fix gnome-screensaver; what I'm doing now is changing the existing gnome-tz-monitor.h in gnome-desktop to be a gnome-wall-clock.h.
Comment 60 Bastien Nocera 2011-09-01 02:27:23 UTC
(In reply to comment #59)
> Sounds fine to me.  In the meantime I have to fix gnome-screensaver; what I'm
> doing now is changing the existing gnome-tz-monitor.h in gnome-desktop to be a
> gnome-wall-clock.h.

You mean the gnome-tz-monitor I removed because nobody used it?
http://git.gnome.org/browse/gnome-desktop/commit/?id=152735fcc4921c83217ab735c316d26bb473539b
Comment 61 Colin Walters 2011-09-01 17:13:54 UTC
GnomeWallClock now lives in https://bugzilla.gnome.org/show_bug.cgi?id=657955
Comment 62 Volker Sobek (weld) 2012-11-29 15:53:25 UTC
Any news here? Will GDateTimeSource get introspection?
Comment 63 Volker Sobek (weld) 2012-11-29 16:44:02 UTC
(In reply to comment #62)
> Will GDateTimeSource get introspection?

Oops, sorry for the noise, I hadn't understood that the functionality I was looking for is already available via gi: I can create a WallClock and connect to it for changes. Nice!
Comment 64 Jasper St. Pierre (not reading bugmail) 2014-05-01 22:21:40 UTC
I don't think we want this in glib anymore, do we? Let's close this.