mirror of
https://github.com/systemd/systemd.git
synced 2026-08-09 09:32:04 +00:00
core/dbus: do not block the manager on GetId during bus (re-)connection
bus_init_api() issued a synchronous GetId call on every API bus (re-)connection to decide whether saved subscription state could be coldplugged onto the new connection. If the D-Bus socket unit is listening while the message bus daemon behind it is gone, connect() succeeds against the socket backlog but nothing answers the authentication handshake. The synchronous call then blocks PID 1 for BUS_AUTH_TIMEOUT (90 seconds by default), and queued bus operations can trigger repeated reconnection attempts. This was observed during shutdown as roughly 15 minutes of teardown progressing only in 90-second intervals. Query the instance ID asynchronously on every connection. Defer API setup until the reply is processed, so saved subscriptions are validated and coldplugged before new subscription requests can arrive. If the query cannot be queued or its reply is invalid, discard the unvalidated state and expose the API without blocking the manager. Reset the live bus ID on every connection and serialize pending bus ID and subscription state across reload and reexec. During daemon-reload, preserve state that was already awaiting the asynchronous reply while discarding the duplicate state produced by the reload itself. Also remove the now-unused synchronous bus_get_instance_id() helper.
This commit is contained in:
105
src/core/dbus.c
105
src/core/dbus.c
@@ -817,6 +817,68 @@ static int bus_setup_api(Manager *m, sd_bus *bus) {
|
|||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
static int api_bus_instance_id_reply(sd_bus_message *reply, void *userdata, sd_bus_error *reterr_error) {
|
||||||
|
Manager *m = ASSERT_PTR(userdata);
|
||||||
|
const sd_bus_error *e;
|
||||||
|
sd_bus *bus;
|
||||||
|
const char *id;
|
||||||
|
int r;
|
||||||
|
|
||||||
|
assert(reply);
|
||||||
|
assert_se(bus = sd_bus_message_get_bus(reply));
|
||||||
|
|
||||||
|
/* Whatever the outcome, we get only one shot at coldplugging the saved subscription state onto
|
||||||
|
* this connection: consume it. */
|
||||||
|
_cleanup_strv_free_ char **subscribed_as_strv = TAKE_PTR(m->subscribed_as_strv);
|
||||||
|
sd_id128_t deserialized_bus_id = m->deserialized_bus_id;
|
||||||
|
m->deserialized_bus_id = SD_ID128_NULL;
|
||||||
|
|
||||||
|
e = sd_bus_message_get_error(reply);
|
||||||
|
if (e) {
|
||||||
|
r = sd_bus_error_get_errno(e);
|
||||||
|
log_full_errno(subscribed_as_strv ? LOG_WARNING : LOG_DEBUG, r,
|
||||||
|
"Failed to query API bus instance ID%s: %s",
|
||||||
|
subscribed_as_strv ? ", not deserializing subscriptions" : ", ignoring",
|
||||||
|
bus_error_message(e, r));
|
||||||
|
goto setup_api;
|
||||||
|
}
|
||||||
|
|
||||||
|
r = sd_bus_message_read_basic(reply, 's', &id);
|
||||||
|
if (r < 0) {
|
||||||
|
log_full_errno(subscribed_as_strv ? LOG_WARNING : LOG_DEBUG, r,
|
||||||
|
"Failed to read API bus instance ID%s: %m",
|
||||||
|
subscribed_as_strv ? ", not deserializing subscriptions" : ", ignoring");
|
||||||
|
goto setup_api;
|
||||||
|
}
|
||||||
|
|
||||||
|
r = sd_id128_from_string(id, &m->bus_id);
|
||||||
|
if (r < 0) {
|
||||||
|
log_full_errno(subscribed_as_strv ? LOG_WARNING : LOG_DEBUG, r,
|
||||||
|
"API bus instance ID not in expected format%s: %m",
|
||||||
|
subscribed_as_strv ? ", not deserializing subscriptions" : ", ignoring");
|
||||||
|
goto setup_api;
|
||||||
|
}
|
||||||
|
|
||||||
|
/* Coldplug the subscriptions deserialized from a previous reload/reexec (or saved when the
|
||||||
|
* previous connection was torn down), but only onto the same bus instance they were recorded on.
|
||||||
|
* If no instance ID was recorded, retain the historical behavior and coldplug without comparison. */
|
||||||
|
if (sd_id128_is_null(deserialized_bus_id) || sd_id128_equal(m->bus_id, deserialized_bus_id))
|
||||||
|
(void) bus_track_coldplug(bus, &m->subscribed, subscribed_as_strv);
|
||||||
|
|
||||||
|
setup_api:
|
||||||
|
r = bus_setup_api(m, bus);
|
||||||
|
if (r < 0) {
|
||||||
|
log_error_errno(r, "Failed to set up API bus: %m");
|
||||||
|
|
||||||
|
if (m->system_bus == bus)
|
||||||
|
bus_done_system(m);
|
||||||
|
if (m->api_bus == bus)
|
||||||
|
bus_done_api(m);
|
||||||
|
}
|
||||||
|
|
||||||
|
return 0;
|
||||||
|
}
|
||||||
|
|
||||||
int bus_init_api(Manager *m) {
|
int bus_init_api(Manager *m) {
|
||||||
_cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL;
|
_cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL;
|
||||||
int r;
|
int r;
|
||||||
@@ -846,17 +908,40 @@ int bus_init_api(Manager *m) {
|
|||||||
return r;
|
return r;
|
||||||
}
|
}
|
||||||
|
|
||||||
r = bus_setup_api(m, bus);
|
/* Query the bus instance ID asynchronously, and let the reply handler decide whether the
|
||||||
if (r < 0)
|
* subscription state deserialized from a previous reload/reexec (or saved when the previous
|
||||||
return log_error_errno(r, "Failed to set up API bus: %m");
|
* connection was torn down) may be coldplugged onto this connection. Set up the API only after the
|
||||||
|
* reply is processed, so new subscriptions cannot be mixed with the pending state before its bus
|
||||||
|
* instance is validated. A synchronous call is not OK here: this is reached on every
|
||||||
|
* (re-)connection attempt, and if the D-Bus socket unit is
|
||||||
|
* listening while the message bus daemon behind it is gone (e.g. it crashed during shutdown while
|
||||||
|
* we still consider its service to be up), a synchronous call over the never-answered connection
|
||||||
|
* blocks the whole manager for BUS_AUTH_TIMEOUT — repeatedly, since every queued bus operation
|
||||||
|
* triggers another reconnection attempt. The previous connection's instance ID is meaningless for
|
||||||
|
* this connection, hence reset it: only ever populate the live ID from the reply, while any pending
|
||||||
|
* validation ID and subscription state remain serializable across a racing reload/reexec. */
|
||||||
|
m->bus_id = SD_ID128_NULL;
|
||||||
|
r = sd_bus_call_method_async(
|
||||||
|
bus,
|
||||||
|
/* ret_slot= */ NULL,
|
||||||
|
"org.freedesktop.DBus",
|
||||||
|
"/org/freedesktop/DBus",
|
||||||
|
"org.freedesktop.DBus",
|
||||||
|
"GetId",
|
||||||
|
api_bus_instance_id_reply,
|
||||||
|
m,
|
||||||
|
/* types= */ NULL);
|
||||||
|
if (r < 0) {
|
||||||
|
log_full_errno(m->subscribed_as_strv ? LOG_WARNING : LOG_DEBUG, r,
|
||||||
|
"Failed to enqueue API bus instance ID query%s: %m",
|
||||||
|
m->subscribed_as_strv ? ", not deserializing subscriptions" : ", ignoring");
|
||||||
|
m->subscribed_as_strv = strv_free(m->subscribed_as_strv);
|
||||||
|
m->deserialized_bus_id = SD_ID128_NULL;
|
||||||
|
|
||||||
r = bus_get_instance_id(bus, &m->bus_id);
|
r = bus_setup_api(m, bus);
|
||||||
if (r < 0)
|
if (r < 0)
|
||||||
log_warning_errno(r, "Failed to query API bus instance ID, not deserializing subscriptions: %m");
|
return log_error_errno(r, "Failed to set up API bus: %m");
|
||||||
else if (sd_id128_is_null(m->deserialized_bus_id) || sd_id128_equal(m->bus_id, m->deserialized_bus_id))
|
}
|
||||||
(void) bus_track_coldplug(bus, &m->subscribed, m->subscribed_as_strv);
|
|
||||||
m->subscribed_as_strv = strv_free(m->subscribed_as_strv);
|
|
||||||
m->deserialized_bus_id = SD_ID128_NULL;
|
|
||||||
|
|
||||||
m->api_bus = TAKE_PTR(bus);
|
m->api_bus = TAKE_PTR(bus);
|
||||||
return 0;
|
return 0;
|
||||||
|
|||||||
@@ -200,8 +200,10 @@ int manager_serialize(
|
|||||||
(void) serialize_ratelimit(f, "reload-reexec-ratelimit", &m->reload_reexec_ratelimit);
|
(void) serialize_ratelimit(f, "reload-reexec-ratelimit", &m->reload_reexec_ratelimit);
|
||||||
(void) serialize_ratelimit(f, "event-loop-ratelimit", &m->event_loop_ratelimit);
|
(void) serialize_ratelimit(f, "event-loop-ratelimit", &m->event_loop_ratelimit);
|
||||||
|
|
||||||
(void) serialize_id128(f, "bus-id", m->bus_id);
|
(void) serialize_id128(f, "bus-id",
|
||||||
|
sd_id128_is_null(m->bus_id) ? m->deserialized_bus_id : m->bus_id);
|
||||||
bus_track_serialize(m->subscribed, f, "subscribed");
|
bus_track_serialize(m->subscribed, f, "subscribed");
|
||||||
|
(void) serialize_strv(f, "subscribed", m->subscribed_as_strv);
|
||||||
|
|
||||||
r = dynamic_user_serialize(m, f, fds);
|
r = dynamic_user_serialize(m, f, fds);
|
||||||
if (r < 0)
|
if (r < 0)
|
||||||
|
|||||||
@@ -3917,8 +3917,10 @@ int manager_override_watchdog_pretimeout_governor(Manager *m, const char *govern
|
|||||||
|
|
||||||
int manager_reload(Manager *m) {
|
int manager_reload(Manager *m) {
|
||||||
_unused_ _cleanup_(manager_reloading_stopp) Manager *reloading = NULL;
|
_unused_ _cleanup_(manager_reloading_stopp) Manager *reloading = NULL;
|
||||||
|
_cleanup_strv_free_ char **saved_subscribed_as_strv = NULL;
|
||||||
_cleanup_fdset_free_ FDSet *fds = NULL;
|
_cleanup_fdset_free_ FDSet *fds = NULL;
|
||||||
_cleanup_fclose_ FILE *f = NULL;
|
_cleanup_fclose_ FILE *f = NULL;
|
||||||
|
sd_id128_t saved_deserialized_bus_id;
|
||||||
int r;
|
int r;
|
||||||
|
|
||||||
assert(m);
|
assert(m);
|
||||||
@@ -3974,6 +3976,10 @@ int manager_reload(Manager *m) {
|
|||||||
manager_enumerate(m);
|
manager_enumerate(m);
|
||||||
|
|
||||||
/* Second, deserialize our stored data */
|
/* Second, deserialize our stored data */
|
||||||
|
saved_subscribed_as_strv = TAKE_PTR(m->subscribed_as_strv);
|
||||||
|
saved_deserialized_bus_id = m->deserialized_bus_id;
|
||||||
|
m->deserialized_bus_id = SD_ID128_NULL;
|
||||||
|
|
||||||
r = manager_deserialize(m, f, fds);
|
r = manager_deserialize(m, f, fds);
|
||||||
if (r < 0)
|
if (r < 0)
|
||||||
log_warning_errno(r, "Deserialization failed, proceeding anyway: %m");
|
log_warning_errno(r, "Deserialization failed, proceeding anyway: %m");
|
||||||
@@ -3987,10 +3993,12 @@ int manager_reload(Manager *m) {
|
|||||||
(void) manager_setup_handoff_timestamp_fd(m);
|
(void) manager_setup_handoff_timestamp_fd(m);
|
||||||
(void) manager_setup_pidref_transport_fd(m);
|
(void) manager_setup_pidref_transport_fd(m);
|
||||||
|
|
||||||
/* Clean up deserialized bus track information. They're never consumed during reload (as opposed to
|
/* Discard the bus track information produced by this reload, since the bus stays connected. Preserve
|
||||||
* reexec) since we do not disconnect from the bus. */
|
* any validation state that was already pending before the reload, so its asynchronous GetId reply can
|
||||||
|
* still consume it when we return to the event loop. */
|
||||||
m->subscribed_as_strv = strv_free(m->subscribed_as_strv);
|
m->subscribed_as_strv = strv_free(m->subscribed_as_strv);
|
||||||
m->deserialized_bus_id = SD_ID128_NULL;
|
m->subscribed_as_strv = TAKE_PTR(saved_subscribed_as_strv);
|
||||||
|
m->deserialized_bus_id = saved_deserialized_bus_id;
|
||||||
|
|
||||||
/* Third, fire things up! */
|
/* Third, fire things up! */
|
||||||
manager_coldplug(m);
|
manager_coldplug(m);
|
||||||
|
|||||||
@@ -10,7 +10,6 @@
|
|||||||
#include "sd-bus.h"
|
#include "sd-bus.h"
|
||||||
#include "sd-daemon.h"
|
#include "sd-daemon.h"
|
||||||
#include "sd-event.h"
|
#include "sd-event.h"
|
||||||
#include "sd-id128.h"
|
|
||||||
|
|
||||||
#include "alloc-util.h"
|
#include "alloc-util.h"
|
||||||
#include "bus-common-errors.h"
|
#include "bus-common-errors.h"
|
||||||
@@ -916,28 +915,6 @@ int bus_query_sender_pidref(
|
|||||||
return bus_creds_get_pidref(creds, ret);
|
return bus_creds_get_pidref(creds, ret);
|
||||||
}
|
}
|
||||||
|
|
||||||
int bus_get_instance_id(sd_bus *bus, sd_id128_t *ret) {
|
|
||||||
_cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
|
|
||||||
int r;
|
|
||||||
|
|
||||||
assert(bus);
|
|
||||||
assert(ret);
|
|
||||||
|
|
||||||
r = sd_bus_call_method(bus,
|
|
||||||
"org.freedesktop.DBus", "/org/freedesktop/DBus", "org.freedesktop.DBus", "GetId",
|
|
||||||
/* reterr_error= */ NULL, &reply, NULL);
|
|
||||||
if (r < 0)
|
|
||||||
return r;
|
|
||||||
|
|
||||||
const char *id;
|
|
||||||
|
|
||||||
r = sd_bus_message_read_basic(reply, 's', &id);
|
|
||||||
if (r < 0)
|
|
||||||
return r;
|
|
||||||
|
|
||||||
return sd_id128_from_string(id, ret);
|
|
||||||
}
|
|
||||||
|
|
||||||
static const char* const bus_transport_table[] = {
|
static const char* const bus_transport_table[] = {
|
||||||
[BUS_TRANSPORT_LOCAL] = "local",
|
[BUS_TRANSPORT_LOCAL] = "local",
|
||||||
[BUS_TRANSPORT_REMOTE] = "remote",
|
[BUS_TRANSPORT_REMOTE] = "remote",
|
||||||
|
|||||||
@@ -70,6 +70,4 @@ int bus_register_malloc_status(sd_bus *bus, const char *destination);
|
|||||||
int bus_creds_get_pidref(sd_bus_creds *c, PidRef *ret);
|
int bus_creds_get_pidref(sd_bus_creds *c, PidRef *ret);
|
||||||
int bus_query_sender_pidref(sd_bus_message *m, PidRef *ret);
|
int bus_query_sender_pidref(sd_bus_message *m, PidRef *ret);
|
||||||
|
|
||||||
int bus_get_instance_id(sd_bus *bus, sd_id128_t *ret);
|
|
||||||
|
|
||||||
DECLARE_STRING_TABLE_LOOKUP_TO_STRING(bus_transport, BusTransport);
|
DECLARE_STRING_TABLE_LOOKUP_TO_STRING(bus_transport, BusTransport);
|
||||||
|
|||||||
Reference in New Issue
Block a user