Bug 25624

Summary: Use monotonic clock for _dbus_get_current_time() if it's available
Product: dbus Reporter: Tom Hughes <tom.hughes>
Component: coreAssignee: Thiago Macieira <thiago>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: walters
Version: 1.3.x (devel)   
Hardware: Other   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: Use monotonic clock for _dbus_get_current_time() if it's available

Description Tom Hughes 2009-12-13 13:46:13 UTC
Created attachment 32048 [details] [review]
Use monotonic clock for _dbus_get_current_time() if it's available

_dbus_get_current_time() is used for timeouts (e.g., in _dbus_connection_block_pending_call()), but uses gettimeofday(), which relies on the wall clock time, which can change. If the time is changed forwards or backwards, the timeouts are no longer valid, so the monotonic clock must be used.

Attached is a patch to use the monotonic clock if it's available.
Comment 1 Thiago Macieira 2009-12-13 14:47:41 UTC
Tom: thanks. This patch is smaller than what you had posted on the mailing list. Can you check if you attached the proper thing?
Comment 2 Tom Hughes 2009-12-13 15:06:43 UTC
I left out the part to pthread_cond_timedwait() because that change has already been made with commit ae24bb35e2ee3ecde990f55852982b573754ec43.

Comment 3 Thiago Macieira 2009-12-13 23:41:12 UTC
Understood.
Comment 4 Colin Walters 2010-02-01 14:58:01 UTC
I'd like to take this next stable cycle, two quick questions:

* Is there any precedent for other projects in the OS core using the monotonic clock?  If so code references would be useful.  I think I saw a patch to glib go by, will have to investigate that.
* Have you looked at all the callers of _dbus_get_current_time?  It looks in a quick glance that all of them should be using this but needs solid investigation.
Comment 5 Tom Hughes 2010-02-01 20:33:43 UTC
> * Is there any precedent for other projects in the OS core using the monotonic
> clock?  If so code references would be useful.  I think I saw a patch to glib
> go by, will have to investigate that.

I'm not sure offhand; a lot of core components are doing the same thing as dbus.

> * Have you looked at all the callers of _dbus_get_current_time?  It looks in a
> quick glance that all of them should be using this but needs solid
> investigation.

One thing to double-check is whether the functions that use _dbus_get_current_time expect the time to include the time that passed while suspended. With Linux (at least 2.6.24), the time spent suspended is not added to the monotonic clock. I'm not sure what POSIX says about this, so I don't know how other platforms behave.


Comment 6 Thiago Macieira 2010-02-02 03:15:07 UTC
Qt's timers are all monotonic-based, even when linking to Glib and using its event loop.

There are some places where the usual gettimeofday() timing is used because of convenience (the QTime class), but it's something I want to fix to use the monotonic timer too.

I think that convention is that if you had a timer that hadn't expired before suspending, when you resume, it continues not expired. The reverse of that would be that many applications get hundreds of expired timers when the system resumes from suspend and eventually try to execute too much at that time.
Comment 7 Colin Walters 2010-02-02 07:10:26 UTC
Ok I looked through all the callers of _dbus_get_current_time more carefully, and using the monotonic clock is either clearly right (connection timeouts) or moot (generating a uuid).

For general reference there's a patch to GLib to use a monotonic clock for timeouts here:

https://bugzilla.gnome.org/show_bug.cgi?id=540545

Thiago, completely agree about the suspend behavior.

I'm pushing this patch to dbus-1.2 now.  Thanks a lot for the patch Tom!


Comment 8 Colin Walters 2010-02-02 07:20:48 UTC
Two small changes; first I changed the #ifdef section to just be "#ifdef HAVE_MONOTONIC_CLOCK" since we already define that in configure.ac, and this also matches the GLib patch. 

Second I added a space before the parens.
Comment 9 Thiago Macieira 2010-02-02 08:08:34 UTC
Now it's too late, but I was going to suggest that this is an invasive change and should go into master only (not 1.2).
Comment 10 Colin Walters 2010-02-02 08:11:46 UTC
Agree it's a big change - however, it is really important for reliability (everyone suspends their linux laptops), it's been running in the field on several linux-based operating systems already, so I decided it's worth taking in 1.2.
Comment 11 John (J5) Palmieri 2010-02-02 09:16:49 UTC
The question to ask is if this change will change API/ABI or behavior someone is expecting.  Since clearly we expect timers to not expire when a user changes the system clock or wake up from a suspend it should go into stable.  It is a bug fix, not a whole new feature.  Now if there is some side effect that will break major D-Bus users there might be consideration to keep it out of stable but if anyone is actually relying on timeouts reacting to users changing the time, those apps are clearly broken and should be fixed.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.