Bug 65134

I can't help thinking that this is a lot of code, and if libdbus was better-designed, it wouldn't be needed...

I'll review this when I can, but I'm afraid I don't have time to swap libdbus' bad GLib reimplementation back into short-term memory right now :-)

Possible workaround 1: don't use dbus_connection_read_write_dispatch(). Instead, integrate libdbus into your mail loop (like dbus-glib and QtDBus do).

Possible workaround 2: use GDBus, part of GIO (distributed with GLib).

(In reply to comment #0)
> The following code reliably reproduces the error (the test uses the google
> test framework, https://code.google.com/p/googletest/)

In general, libdbus doesn't use C++, but if someone ported this to something used by libdbus (either its own test infrastructure as used in test/name-test/, or the GLib test framework as used in test/dbus-daemon.c) then it could be included with libdbus as a regression test.

(In reply to comment #0)
> Reason is that during the iteration those timeouts are
> never checked again.

Ah, actually, I see the problem here: it is that dbus_connection_read_write_dispatch() has no concept of timeouts.

> Optimal in our opinion would be the
> extension of the DBusPendingCall by a member variable that remembers the
> time the call was issued, and to check the current time against this value
> plus the timeout interval, instead of relying on the timeout appended to a
> DBusPendingCall firing only once and using the "interval" member variable of
> this timeout object for the very same purpose.

That would fix this for dbus_connection_read_write_dispatch(), but would break "normal" use of libdbus (in conjunction with a separate main loop).

The reason your patch is so large is that you're implementing about half of a main loop... but that's probably the only way to fix this.

Possible resolutions are basically "review and improve your patch" or "document how broken dbus_connection_read_write_dispatch() is, and deprecate it" :-(

Comment on attachment 79978 [details] [review]
Possible fix for dbus-1.6.11

Review of attachment 79978 [details] [review]:
-----------------------------------------------------------------

This is just a quick skim-read, I'll come back to this in more detail when I can.

Commit message: git commit messages are conventionally a short one-line "subject", followed by a blank line, followed by more explanation. For instance see <http://ablogaboutcode.com/2011/03/23/proper-git-commit-messages-and-an-elegant-git-history/>

::: dbus/dbus-connection.c
@@ +3615,4 @@
>    _dbus_verbose ("end\n");
>  }
>  
> +static int _timeouts_check(DBusConnection *connection, int timeout_milliseconds) {

This is meant to be called with the lock held, so it should be called something_unlocked(). Bonus points if you add a comment saying *which* lock (in this case, connection's lock).

This would benefit from a doc-comment mentioning that it returns a revised timeout equal to or less than timeout_milliseconds.

Please follow the existing, coding style, which is approximately:

* GNU-style indentation
* newline between function definition's type and name
* space before opening parenthesis

static int
_timeouts_check_unlocked (DBusConnection *connection,
                          int timeout_milliseconds)
{
  ...

  while (_dbus_hash_iter_next (&iter))
    {
      blah blah blah;
    }
}

@@ +3618,5 @@
> +static int _timeouts_check(DBusConnection *connection, int timeout_milliseconds) {
> +    int timeout_interval;
> +    DBusHashIter iter;
> +    DBusPendingCall *pending;
> +    DBusTimeout* timeout;

Coding style: we consistently use "Struct *pointer", not "Struct* pointer" (so that the appearance of the code matches C's precedence semantics).

@@ +3632,5 @@
> +            if(dbus_timeout_get_enabled(timeout)) {
> +                long current_tv_sec, current_tv_usec;
> +                _dbus_get_monotonic_time(&current_tv_sec, &current_tv_usec);
> +                timeout_interval = dbus_timeout_get_interval(timeout);
> +                timeout_milliseconds = timeout_milliseconds < timeout_interval ? timeout_milliseconds : timeout_interval;

I think we have a MIN() macro you can use.

@@ +3646,5 @@
> +    return timeout_milliseconds;
> +}
> +
> +static void _timeouts_handle_helper(void *element, void *data) {
> +    dbus_timeout_handle((DBusTimeout*) element);

Coercion from (void *) to (Thing *) doesn't require a cast in C.

@@ +3669,5 @@
> +            timeout = _dbus_pending_call_get_timeout_unlocked(pending);
> +            timeout_interval = dbus_timeout_get_interval(timeout);
> +            if (timeout && dbus_timeout_get_enabled(timeout)) {
> +                if (iteration_time_millis >= timeout_interval) {
> +                    _dbus_list_append(&timeout_list, timeout);

This can fail, on OOM. Please handle it (probably by returning "not enough memory" from ..._read_write_dispatch()).

@@ +3671,5 @@
> +            if (timeout && dbus_timeout_get_enabled(timeout)) {
> +                if (iteration_time_millis >= timeout_interval) {
> +                    _dbus_list_append(&timeout_list, timeout);
> +                } else {
> +                    _dbus_timeout_set_interval(timeout, timeout_interval - iteration_time_millis);

Ugh. This happens to be OK for a "one-shot" timeout that will only be used with ...read_write_dispatch(), but would be wrong when integrating with a main loop, because main loop implementations are allowed to assume that a timeout's interval never changes...

@@ +3677,5 @@
> +            }
> +        }
> +    }
> +
> +    _dbus_list_foreach(&timeout_list, _timeouts_handle_helper, NULL);

I usually find it's clearer to do iteration explicitly:

    DBusList *link;
    ...
    for (link = _dbus_list_get_first_link (&timeout_list);
         link != NULL);
         link = _dbus_list_get_next_link (&timeout_list, link))
      dbus_timeout_handle (link->data);

@@ +3679,5 @@
> +    }
> +
> +    _dbus_list_foreach(&timeout_list, _timeouts_handle_helper, NULL);
> +
> +    _dbus_list_clear(&timeout_list);

You could even combine this with the iteration, by doing destructive iteration:

    while ((timeout = _dbus_list_pop_first (&timeout_list)) != NULL)
      dbus_timeout_handle (timeout);

I think this would work better with a design like this:

* Have a queue of pending calls, in order of monotonic expiry time

* Store a monotonic expiry time either in each pending call, or in structs { pending call, expiry time } in that queue?

* Maybe don't build that queue for the first time until someone calls read_write_dispatch(), so that "normal use" with a main loop doesn't need to track it?

* Limit the timeout of dbus_connection_read_write_dispatch() to the time remaining until the first thing in that queue

* In read_write_dispatch(), after time has passed, go through that queue in order: for each pending call, if it has expired, trigger the callback; if it has not, because they're in order, nothing else can have expired either, so stop

Ok, thank you very much for the thorough feedback. I'll try to incorporate the ideas into the patch once I find some time to do so (unless someone else is faster  than I am and does so before me :) )

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/83.