picnoir
/
Systemd
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

sd-bus: start reply callback timeouts only when the connection is established 

Currently, reply callback timeouts are started the instant the method
calls are enqueued, which can be very early on. For example, the Hello()
method call is enqueued right when sd_bus_start() is called, i.e. before
the socket connection and everything is established.

With this change we instead start the method timeout the moment we
actually leave the authentication phase of the connection. This way, the
timeout the kernel applies on socket connecting, and we apply on the
authentication phase no longer runs in parallel to the Hello() method
call, but all three run serially one after the other, which is
definitely a cleaner approach.

Moreover, this makes the "watch bind" feature a lot more useful, as it
allows enqueuing method calls while we are still waiting for inotify
events, without them timeouting until the connection is actually
established, i.e. when the method call actually has a chance of being
actually run.

This is a change of behaviour of course, but I think the new behaviour
is much better than the old one, since we don't race timeouts against
each other anymore...
This commit is contained in:
Lennart Poettering 2017-12-18 13:53:12 +01:00
parent 8a5cd31e5f
commit ac8029fc25
3 changed files with 51 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/libsystemd/sd-bus/bus-internal.h
View File

@ -38,7 +38,7 @@
struct reply_callback {
sd_bus_message_handler_t callback;
usec_t timeout;
usec_t timeout_usec; /* this is a relative timeout until we reach the BUS_HELLO state, and an absolute one right after */
uint64_t cookie;
unsigned prioq_idx;
};
@ -157,10 +157,10 @@ struct sd_bus_slot {
enum bus_state {
BUS_UNSET,
BUS_WATCH_BIND, /* waiting for the socket to appear via inotify */
BUS_OPENING,
BUS_AUTHENTICATING,
BUS_HELLO,
BUS_WATCH_BIND, /* waiting for the socket to appear via inotify */
BUS_OPENING, /* the kernel's connect() is still not ready */
BUS_AUTHENTICATING, /* we are currently in the "SASL" authorization phase of dbus */
BUS_HELLO, /* we are waiting for the Hello() response */
BUS_RUNNING,
BUS_CLOSING,
BUS_CLOSED

2
src/libsystemd/sd-bus/bus-slot.c
View File

@ -81,7 +81,7 @@ void bus_slot_disconnect(sd_bus_slot *slot) {
if (slot->reply_callback.cookie != 0)
ordered_hashmap_remove(slot->bus->reply_callbacks, &slot->reply_callback.cookie);
if (slot->reply_callback.timeout != 0)
if (slot->reply_callback.timeout_usec != 0)
prioq_remove(slot->bus->reply_callbacks_prioq, &slot->reply_callback, &slot->reply_callback.prioq_idx);
break;

63
src/libsystemd/sd-bus/sd-bus.c
View File

@ -461,7 +461,24 @@ static int bus_send_hello(sd_bus *bus) {
}
int bus_start_running(sd_bus *bus) {
struct reply_callback *c;
Iterator i;
usec_t n;
assert(bus);
assert(bus->state < BUS_HELLO);
/* We start all method call timeouts when we enter BUS_HELLO or BUS_RUNNING mode. At this point let's convert
* all relative to absolute timestamps. Note that we do not reshuffle the reply callback priority queue since
* adding a fixed value to all entries should not alter the internal order. */
n = now(CLOCK_MONOTONIC);
ORDERED_HASHMAP_FOREACH(c, bus->reply_callbacks, i) {
if (c->timeout_usec == 0)
continue;
c->timeout_usec = usec_add(n, c->timeout_usec);
}
if (bus->bus_client) {
bus->state = BUS_HELLO;
@ -1721,26 +1738,35 @@ _public_ int sd_bus_send_to(sd_bus *bus, sd_bus_message *m, const char *destinat
return sd_bus_send(bus, m, cookie);
}
static usec_t calc_elapse(uint64_t usec) {
static usec_t calc_elapse(sd_bus *bus, uint64_t usec) {
assert(bus);
if (usec == (uint64_t) -1)
return 0;
return now(CLOCK_MONOTONIC) + usec;
/* We start all timeouts the instant we enter BUS_HELLO/BUS_RUNNING state, so that the don't run in parallel
* with any connection setup states. Hence, if a method callback is started earlier than that we just store the
* relative timestamp, and afterwards the absolute one. */
if (IN_SET(bus->state, BUS_WATCH_BIND, BUS_OPENING, BUS_AUTHENTICATING))
return usec;
else
return now(CLOCK_MONOTONIC) + usec;
}
static int timeout_compare(const void *a, const void *b) {
const struct reply_callback *x = a, *y = b;
if (x->timeout != 0 && y->timeout == 0)
if (x->timeout_usec != 0 && y->timeout_usec == 0)
return -1;
if (x->timeout == 0 && y->timeout != 0)
if (x->timeout_usec == 0 && y->timeout_usec != 0)
return 1;
if (x->timeout < y->timeout)
if (x->timeout_usec < y->timeout_usec)
return -1;
if (x->timeout > y->timeout)
if (x->timeout_usec > y->timeout_usec)
return 1;
return 0;
@ -1800,11 +1826,11 @@ _public_ int sd_bus_call_async(
return r;
}
s->reply_callback.timeout = calc_elapse(m->timeout);
if (s->reply_callback.timeout != 0) {
s->reply_callback.timeout_usec = calc_elapse(bus, m->timeout);
if (s->reply_callback.timeout_usec != 0) {
r = prioq_put(bus->reply_callbacks_prioq, &s->reply_callback, &s->reply_callback.prioq_idx);
if (r < 0) {
s->reply_callback.timeout = 0;
s->reply_callback.timeout_usec = 0;
return r;
}
}
@ -1891,7 +1917,7 @@ _public_ int sd_bus_call(
if (r < 0)
goto fail;
timeout = calc_elapse(m->timeout);
timeout = calc_elapse(bus, m->timeout);
for (;;) {
usec_t left;
@ -2099,12 +2125,12 @@ _public_ int sd_bus_get_timeout(sd_bus *bus, uint64_t *timeout_usec) {
return 0;
}
if (c->timeout == 0) {
if (c->timeout_usec == 0) {
*timeout_usec = (uint64_t) -1;
return 0;
}
*timeout_usec = c->timeout;
*timeout_usec = c->timeout_usec;
return 1;
case BUS_CLOSING:
@ -2131,13 +2157,14 @@ static int process_timeout(sd_bus *bus) {
int r;
assert(bus);
assert(IN_SET(bus->state, BUS_RUNNING, BUS_HELLO));
c = prioq_peek(bus->reply_callbacks_prioq);
if (!c)
return 0;
n = now(CLOCK_MONOTONIC);
if (c->timeout > n)
if (c->timeout_usec > n)
return 0;
r = bus_message_new_synthetic_error(
@ -2153,7 +2180,7 @@ static int process_timeout(sd_bus *bus) {
return r;
assert_se(prioq_pop(bus->reply_callbacks_prioq) == c);
c->timeout = 0;
c->timeout_usec = 0;
ordered_hashmap_remove(bus->reply_callbacks, &c->cookie);
c->cookie = 0;
@ -2264,9 +2291,9 @@ static int process_reply(sd_bus *bus, sd_bus_message *m) {
return r;
}
if (c->timeout != 0) {
if (c->timeout_usec != 0) {
prioq_remove(bus->reply_callbacks_prioq, c, &c->prioq_idx);
c->timeout = 0;
c->timeout_usec = 0;
}
is_hello = bus->state == BUS_HELLO && c->callback == hello_callback;
@ -2602,9 +2629,9 @@ static int process_closing_reply_callback(sd_bus *bus, struct reply_callback *c)
if (r < 0)
return r;
if (c->timeout != 0) {
if (c->timeout_usec != 0) {
prioq_remove(bus->reply_callbacks_prioq, c, &c->prioq_idx);
c->timeout = 0;
c->timeout_usec = 0;
}
ordered_hashmap_remove(bus->reply_callbacks, &c->cookie);