GNOME Bugzilla – Bug 644771
GDateTime: better localtime timezone handling
Last modified: 2011-04-07 21:24:49 UTC
GTimeZone has a few problems with respect to handling of the local timezone: - if the last refcount on the local GTimeZone drops, /etc/localtime is closed - on next use, it is reopened - this can result in open("/etc/localtime") happening once per second under certain use cases - if someone else happens to hold on to the local GTimeZone in the same process then the behaviour magically changes to not noticing timezone changes at all There are some complicating factors: - we have no way within glib itself to monitor for changes (since the file notification API is not part of libglib, but rather libgio) - GDateTime is immutable, so it would be weird if we saw a GDateTime that was created in the local timezone change its time - GDateTime isn't a GObject, so we can't have a 'changed' signal - internally, GDateTime caches the interval in the underlying timezone so we can't let that change without getting GDateTime involved I think the best way forward is this: - start caching the local GTimeZone (and maybe the one for UTC While we're at it) to prevent syscall thrashing - add a g_time_zone_refresh_localtime() API to drop the cache - add a singleton GTimeZoneMonitor to gio with: 1) GTimeZoneMonitor * g_time_zone_monitor_get (void); 2) a 'changed' signal, VOID:VOID This would be implemented by watching /etc/localtime for changes and when one happens, calling g_time_zone_refresh_localtime() and emitting the change signal. - document that people who are interested in noticing timezone changes are responsible for connecting to this signal. Owen mentioned an interesting case: imagine we have a GtkTreeModel that is filled with GDateTime objects in the local timezone and the local timezone changes. Do we really want to force the programmer to recreate all of those objects? Shouldn't they automatically update to show the time in the new timezone? This certainly seems very nice, but it means that some accessors of a supposedly immutable object could start returning different values. So here's where we have two roads: 1) document that GDateTime is immutable in the strictest possible sense and also that performing operations on existing GDateTime objects will keep the timezone in effect when that object was created. If you want to see the new local time, you need to create a fresh one. 2) document that a GDateTime created in the local timezone may change its value in response to the local timezone changing The second option seems more programmer friendly but it has a somewhat large problem: many people call separate APIs of a GDateTime like get_hour() and get_minute() and expect that the object won't change value between those calls. If done in the main thread then this is probably true (since any call to refresh the localtime would probably be made from a mainloop dispatch). GDateTime is threadsafe, though -- so safe access to the object from another thread could become theoretically impossible. Although I think the first option is a bit rough, it may be the only way to go.
We could also advise people to store lists of UTC GDateTime that they do g_date_time_to_local() on just before formatting them for display.
We have a bit of code in gnome-desktop (nicked from the panel) to do this, but it's certainly not good enough for consumption within GLib. See bug 626346.
The timezone monitor seems like an ok idea. For choice 1), if you have a local datetime and you know the timezone might have changed, would it be enough to simply call g_date_time_to_local (dt) to 'refresh' it ?
Created attachment 183772 [details] [review] GTimeZone: don't add /etc/localtime to the cache
Created attachment 183773 [details] [review] g_time_zone_new_local: cache the result
Created attachment 183774 [details] [review] Add GTimeZoneMonitor
I need to change the type of caching that's done because the local timezone is not strictly the same thing as /etc/localtime (since we have the TZ environment variable). Doing the caching explicitly in g_time_zone_new_local() ensures that we don't trash, even in the case that TZ is set. Open question: do we attempt to detect this situation and avoid monitoring /etc/localtime from GTimeZoneMonitor? I doubt it matters.
Review of attachment 183774 [details] [review]: Looks good. See the comment. ::: gio/gtimezonemonitor.c @@ +88,3 @@ + GFile *etc_localtime; + + etc_localtime = g_file_new_for_path ("/etc/localtime"); Should we give people the ability to change that filename through a compile time switch? I guess all sane Unixes use that file but who knows.
Review of attachment 183773 [details] [review]: Looks ok to me.
Ugh. What about moving file monitoring to GLib (as a "private" but exported interface, i.e. _g_internal_monitor_file)? It seems particularly weird to me that simply *instantiating* a GTimezoneMonitor has the side effect of changing all GDateTime instances. Realistically I would expect this to happen *by default*, and then if an app is for some reason interested in time zone changes, it could connect to the monitor. At the present state, we're basically saying GDateTime is broken unless you link to gio too. Which, okay maybe moving monitoring would be difficult, but we should document it as being lame, and something we may fix in the future.
Honestly, I wish we had made GDateTime a real GObject and put it in Gio...but too late =/
Review of attachment 183772 [details] [review]: OK, nice to see the g_str_hash0/g_str_equal0 stuff go away. Looks cleaner now.
Colin: I decided to go the route of not changing any existing GDateTime instances. It's just too weird. All this does is change the timezone that is used for new instances created with the _new_local() variants.
<desrt> walters: hey walters: feedback on the timemonitor stuff (very) welcome <walters> it's an ugly problem for sure <desrt> walters: i'm not changing existing GDateTime instances <desrt> even if you do g_date_time_add_seconds() or something like that, the timezone still won't change only really newly created ones --> stephh (~steph@adsl-71-135-35-253.dsl.pltn13.pacbell.net) has joined #gtk+ <desrt> g_date_time_new_now_local() for example and of course g_date_time_to_local() <walters> but instantating the monitor will have the side effect for new ones, right? <desrt> yes <walters> that's what i mean <desrt> but i have trouble arguing that that's a bad thing... <walters> components (and apps probably) will start getting random static GTimezoneMonitor *mon = g_timezone_monitor_new (); (void) mon; <desrt> this "side effect" would occur in the existing code already as long as nobody happened to have a GTimeZone open already <walters> the (void) mon; trick to pacify gcc's set-but-unused warning <desrt> g_object_unref(g_timezone_monitor_get()); is the most technically-correct way to do that it's a live-forever singleton but i argue that if you're interested in noticing changes to the local timezone then you're interested in actually knowing when those changes happen <walters> okay well, it's gross =) maybe we have to do it but i'd like to try a little harder to think of other ways <desrt> so most sane uses will involve hooking up a signal handler so one thing in particular: we can't just start monitoring from GTimeZone itself even if we had the file notification in glib because otherwise you could only use GDateTime if you had a mainloop which is not currently a requirement <walters> well, i think we could safely say you don't get updates if you don't use a mainloop but GMainLoop *is* in GLib, and is also widely used in pure glib apps <desrt> the problem is that we have no reliable way of detecting if the user intends to run a mainloop so we'd have filesystem notification events coming in and stacking up to dispatch on a never-run main context <walters> hmm, they wouldn't have to stack <desrt> someone has to read() the fd... and if nobody does, they will stack... <walters> true; maybe we special case the internal interface to do the read, and just set a boolean <desrt> we could maybe get away with that for inotify but it might not generally be possible to take that approach on all systems <walters> then gio can, when it initializes, close the fd and re set things up <desrt> and even if we could, i'm not sure i'd want to introduce a second file monitoring abstraction API :) <walters> it would be private also we could say f it, it only works with inotify... <desrt> we could also call stat() a lot <walters> true it's pretty cheap <desrt> (and stop doing that once we have GTimeZoneMonitor existing) <walters> right hmm <desrt> cool <Company> hrm <desrt> add your thoughts to the bug please <walters> but something would still need to initialize the monitor <desrt> i have to admit that i'm not crazy about this API <Company> desrt: are you trying to solve the cornercase of non-mainloop applications needing notifications for changes for timezones? <walters> ideally if your process has gio linked in even if you didn't create any GFileMonitor, your GDateTime instances worked <desrt> it's kinda crappy Company: no. i'm not. --> jpetersen (~jpetersen@brln-4dbc00b0.pool.mediaWays.net) has joined #gtk+ <desrt> Company: that's madness. <walters> i just don't like GTimezoneMonitor <desrt> Company: but i'm trying to make sure i don't accidentally interfere with their desire to *not* receive notification <desrt> walters: *something* needs to emit the signal <desrt> that's necessarily a GObject, in my opinion <Cimi> Company: I noticed that scrolling in evolution's folder sidebar might be slow because it's redrawing a log of tije <walters> hmm, yeah i guess some components and apps may want notification ok <desrt> some weird callback-based interface is a no-go descender desrt <Cimi> *times what's inside the treeview <walters> desrt: yeah i agree it's more the using-it-for-side-effect --> kkris1 (~kris@188-22-124-207.adsl.highway.telekom.at) has joined #gtk+ <Company> Cimi: why would it do that? <desrt> walters: let me put it this way: how would you feel if i removed that side effect and treated these two patches as related but completely separate issues? <Cimi> Company: I noticed because I added a printf to the pager of my overlay scrollbar <desrt> (a) improved caching semantics for local timezone (b) utility class to notice when /etc/localtime changes <Cimi> Company: only in evolution sidebar descender desrt <desrt> that's not so strange, i guess..... <walters> desrt: i like that yes <Cimi> Company: I'm getting dozens of calls during scroll descender desrt <desrt> walters: okay. so far so good. <walters> desrt: mind if i paste the discussion into the bug for historical reference? <desrt> walters: but now that we're this far, why not do the obviously-correct thing and have the monitor notify GTimeZone that it noticed the change? <Cimi> Company: printf to the expose descender desrt <desrt> walters: that's clearly an improvement. walters: yes. of course. i was planning on making a summary, but the log will do nicely. <walters> desrt: well, maybe the first patch should be using stat() by default, until a monitor is created <desrt> walters: it's certainly a possibility. <desrt> i'm a bit worried about using stat() in case we can't notice the change <walters> i feel like we're overengineering a bit, but on the otherhand this is public, permanent API, so <desrt> i can imagine a situation where the new timezone would have the same inode as the old one descender desrt like.... rm localtime (frees up an inode) <walters> desrt: we check the ctime <desrt> cp -a /usr/share/zoneinfo/something localtime (re-use same inode) <desrt> walters: if we do cp -a then the ctime comes from the source <walters> also remember to compare (device, inum) pair descender desrt <desrt> and i just came up with a case where the inode could be the same <walters> desrt: but...if the OS does 'cp -a" then the contents are the same? <Cimi> Company: so I shifted the position of the child window I was drawing on the scrolled window Company: I moved it outside the treeview Company: and it's damn faster <walters> anyways at that level it's just something to ensure the underlying OS does right <desrt> ya. if we decide that we want to stat, i'm *fairly* sure we can figure out something <Company> Cimi: it's not redrawing a lot here <desrt> just have to be very careful and cover all our bases <walters> if Joe Bob's Linux From Scratch doesn't, well screw it <desrt> we may get some bugs.... so i think it's quite ugly that we will have some g_time_zone_stop_the_stat_syscalls() API descender desrt <walters> desrt: i think we can make it "private" right? like exported but _ prefixed etc <desrt> walters: yes. we can do that. <-- kkris has quit (Ping timeout: 600 seconds) <Cimi> Company: so, explain me why if the pager (is a child window) is placed *inside* the treeview the scrolling is damn slow, moving it outside and the speed is blazing fast <desrt> we already have a few symbols like that (gettext utility functions, for example) i don't like how GTimeZoneMonitor would be 'special' in this sense, though <Company> Cimi: i have no idea <desrt> someone may want to do their own monitoring <Cimi> Company: the only Idea I have is invalidating the region Company: with its children <-- thiagoss has quit (Ex-Chat) <Cimi> Company: if there's a bogus code in gtk+, we should fix that <Company> Cimi: indeed <desrt> walters: anyway... pop the conversation into a bug and we'll wait for someone to come along and give a 3rd opinion
I accidentally pushed my patches to git with some other unrelated modifications. I think that was probably the correct thing to do anyway, but maybe we want to talk about some tweaks (the issues Colin brought up, for example).
I'm going to close this bug now (by request of David). We can discuss specific issues in other bugs.
*** Bug 644761 has been marked as a duplicate of this bug. ***