[dconf: 4/5] gdbus: Unref cached GDBusConnection objects when the connection is closed
- From: Daniel Playfair Cal <danielplayfaircal src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [dconf: 4/5] gdbus: Unref cached GDBusConnection objects when the connection is closed
- Date: Thu, 15 Aug 2019 00:19:13 +0000 (UTC)
commit a32e51142140e3886b50ba7348783d867e900d0b
Author: Andre Moreira Magalhaes <andre endlessm com>
Date: Tue Jul 23 20:33:19 2019 +0000
gdbus: Unref cached GDBusConnection objects when the connection is closed
This change fixes the dbus-leak tests by dropping the cached
GDBusConnection objects references when the bus connection is closed.
The issue was introduced with recent changes made to GLib[1]
where invoking g_test_dbus_down() will fail after a
timeout if the GDBusConnection object for the session bus leaks.
Given g_test_dbus_down() will first close the connection before checking
for leaks unreffing the object when the connection is closed should fix
the issue.
[1] https://gitlab.gnome.org/GNOME/glib/merge_requests/963
Signed-off-by: Andre Moreira Magalhaes <andre endlessm com>
engine/dconf-engine.h | 11 ++++
gdbus/dconf-gdbus-common.c | 42 ++++++++++++++
gdbus/dconf-gdbus-filter.c | 133 +++++++++++++++++++++++++++++----------------
gdbus/dconf-gdbus-thread.c | 40 +++++++++++---
gdbus/meson.build | 16 +++++-
5 files changed, 185 insertions(+), 57 deletions(-)
---
diff --git a/engine/dconf-engine.h b/engine/dconf-engine.h
index 2485423..6d57782 100644
--- a/engine/dconf-engine.h
+++ b/engine/dconf-engine.h
@@ -67,6 +67,17 @@ GVariant * dconf_engine_dbus_call_sync_func (GBusTyp
const GVariantType
*expected_type,
GError **error);
+/* Helper function used by the client library to handle bus disconnection */
+G_GNUC_INTERNAL
+void dconf_engine_dbus_handle_connection_closed (GDBusConnection *connection,
+ gboolean
remote_peer_vanished,
+ GError *error,
+ GMutex *bus_lock,
+ gboolean
*bus_is_error,
+ gpointer *bus_data,
+ GCallback
bus_closed_callback,
+ gpointer
bus_closed_callback_user_data);
+
/* Notifies that a change occured.
*
* The engine lock is never held when calling this function so it is
diff --git a/gdbus/dconf-gdbus-common.c b/gdbus/dconf-gdbus-common.c
new file mode 100644
index 0000000..4b1b00c
--- /dev/null
+++ b/gdbus/dconf-gdbus-common.c
@@ -0,0 +1,42 @@
+#include "../engine/dconf-engine.h"
+
+void
+dconf_engine_dbus_handle_connection_closed (GDBusConnection *connection,
+ gboolean remote_peer_vanished,
+ GError *error,
+ GMutex *bus_lock,
+ gboolean *bus_is_error,
+ gpointer *bus_data,
+ GCallback bus_closed_callback,
+ gpointer bus_closed_callback_user_data)
+{
+ g_return_if_fail (connection != NULL);
+ g_return_if_fail (bus_is_error != NULL);
+ g_return_if_fail (bus_data != NULL);
+
+ g_debug ("D-Bus connection closed, invalidating cache: %s",
+ error != NULL ? error->message :
+ (remote_peer_vanished == FALSE ? "Close requested" : "Unknown reason"));
+
+ g_mutex_lock (bus_lock);
+
+ if (bus_closed_callback)
+ g_signal_handlers_disconnect_by_func (connection,
+ bus_closed_callback,
+ bus_closed_callback_user_data);
+
+ if (*bus_is_error)
+ {
+ g_clear_error ((GError **) bus_data);
+ *bus_is_error = FALSE;
+ }
+ else
+ {
+ g_assert (connection == *bus_data);
+ *bus_data = NULL;
+ }
+
+ g_object_unref (connection);
+
+ g_mutex_unlock (bus_lock);
+}
diff --git a/gdbus/dconf-gdbus-filter.c b/gdbus/dconf-gdbus-filter.c
index 79b2dd7..dddc00c 100644
--- a/gdbus/dconf-gdbus-filter.c
+++ b/gdbus/dconf-gdbus-filter.c
@@ -6,7 +6,7 @@
typedef struct
{
gpointer data; /* either GDBusConnection or GError */
- guint is_error;
+ gboolean is_error;
guint waiting_for_serial;
GQueue queue;
} ConnectionState;
@@ -156,6 +156,22 @@ dconf_gdbus_filter_function (GDBusConnection *connection,
return message;
}
+static void
+dconf_gdbus_bus_connection_closed (GDBusConnection *connection,
+ gboolean remote_peer_vanished,
+ GError *error,
+ gpointer user_data)
+{
+ ConnectionState *state = user_data;
+
+ dconf_engine_dbus_handle_connection_closed (connection, remote_peer_vanished, error,
+ &dconf_gdbus_lock,
+ &state->is_error,
+ &state->data,
+ G_CALLBACK (dconf_gdbus_bus_connection_closed),
+ user_data);
+}
+
static ConnectionState *
dconf_gdbus_get_connection_state (GBusType bus_type,
GError **error)
@@ -166,7 +182,7 @@ dconf_gdbus_get_connection_state (GBusType bus_type,
state = &connections[bus_type];
- if (g_once_init_enter (&state->data))
+ if (!state->data)
{
GDBusConnection *connection;
GError *error = NULL;
@@ -180,6 +196,9 @@ dconf_gdbus_get_connection_state (GBusType bus_type,
if (connection)
{
+ g_signal_connect (connection, "closed",
+ G_CALLBACK (dconf_gdbus_bus_connection_closed),
+ state);
g_dbus_connection_add_filter (connection, dconf_gdbus_filter_function, state, NULL);
result = connection;
state->is_error = FALSE;
@@ -190,11 +209,11 @@ dconf_gdbus_get_connection_state (GBusType bus_type,
state->is_error = TRUE;
}
- g_once_init_leave (&state->data, result);
+ state->data = result;
}
if (!connection_state_ensure_success (state, error))
- return FALSE;
+ return NULL;
return state;
}
@@ -213,62 +232,67 @@ dconf_engine_dbus_call_async_func (GBusType bus_type,
GDBusMessage *message;
DConfGDBusCall *call;
gboolean success;
+ volatile guint *serial_ptr;
+ guint my_serial;
+
+ /* Hold the lock to make sure nothing else updates state (e.g. invalidates
+ * connection) while we are processing here.
+ */
+ g_mutex_lock (&dconf_gdbus_lock);
state = dconf_gdbus_get_connection_state (bus_type, error);
if (state == NULL)
{
g_variant_unref (g_variant_ref_sink (parameters));
+
+ g_mutex_unlock (&dconf_gdbus_lock);
+
return FALSE;
}
message = g_dbus_message_new_method_call (bus_name, object_path, interface_name, method_name);
g_dbus_message_set_body (message, parameters);
- g_mutex_lock (&dconf_gdbus_lock);
- {
- volatile guint *serial_ptr;
- guint my_serial;
-
- /* We need to set the serial in call->serial. Sometimes we also
- * need to set it in state->waiting_for_serial (in the case that no
- * other items are queued yet).
- *
- * g_dbus_connection_send_message() only has one out_serial parameter
- * so we can only set one of them atomically. If needed, we elect
- * to set the waiting_for_serial because that is the one that is
- * accessed from the filter function without holding the lock.
- *
- * The serial number in the call structure is only accessed after the
- * lock is acquired which allows us to take our time setting it (for
- * as long as we're still holding the lock).
- *
- * In the case that waiting_for_serial should not be set we just use
- * a local variable and use that to fill call->serial.
- *
- * Also: the queue itself isn't accessed until after the lock is
- * taken, so we can delay adding the call to the queue until we know
- * that the sending of the message was successful.
- */
-
- if (g_queue_is_empty (&state->queue))
- serial_ptr = &state->waiting_for_serial;
- else
- serial_ptr = &my_serial;
-
- success = g_dbus_connection_send_message (connection_state_get_connection (state), message,
- G_DBUS_SEND_MESSAGE_FLAGS_NONE, serial_ptr, error);
+ /* We need to set the serial in call->serial. Sometimes we also
+ * need to set it in state->waiting_for_serial (in the case that no
+ * other items are queued yet).
+ *
+ * g_dbus_connection_send_message() only has one out_serial parameter
+ * so we can only set one of them atomically. If needed, we elect
+ * to set the waiting_for_serial because that is the one that is
+ * accessed from the filter function without holding the lock.
+ *
+ * The serial number in the call structure is only accessed after the
+ * lock is acquired which allows us to take our time setting it (for
+ * as long as we're still holding the lock).
+ *
+ * In the case that waiting_for_serial should not be set we just use
+ * a local variable and use that to fill call->serial.
+ *
+ * Also: the queue itself isn't accessed until after the lock is
+ * taken, so we can delay adding the call to the queue until we know
+ * that the sending of the message was successful.
+ */
+
+ if (g_queue_is_empty (&state->queue))
+ serial_ptr = &state->waiting_for_serial;
+ else
+ serial_ptr = &my_serial;
+
+ success = g_dbus_connection_send_message (connection_state_get_connection (state), message,
+ G_DBUS_SEND_MESSAGE_FLAGS_NONE, serial_ptr, error);
+
+ if (success)
+ {
+ call = g_slice_new (DConfGDBusCall);
- if (success)
- {
- call = g_slice_new (DConfGDBusCall);
+ call->handle = handle;
+ call->serial = *serial_ptr;
- call->handle = handle;
- call->serial = *serial_ptr;
+ g_queue_push_tail (&state->queue, call);
+ }
- g_queue_push_tail (&state->queue, call);
- }
- }
g_mutex_unlock (&dconf_gdbus_lock);
g_object_unref (message);
@@ -286,18 +310,35 @@ dconf_engine_dbus_call_sync_func (GBusType bus_type,
const GVariantType *reply_type,
GError **error)
{
+ g_autoptr(GDBusConnection) connection = NULL;
ConnectionState *state;
+ /* Hold the lock to make sure nothing else updates state (e.g. invalidates
+ * connection) while we are processing here.
+ */
+ g_mutex_lock (&dconf_gdbus_lock);
+
state = dconf_gdbus_get_connection_state (bus_type, error);
if (state == NULL)
{
g_variant_unref (g_variant_ref_sink (parameters));
+ g_mutex_unlock (&dconf_gdbus_lock);
+
return NULL;
}
- return g_dbus_connection_call_sync (connection_state_get_connection (state),
+ /* Hold a ref to the connection to make sure the object is still
+ * valid even if we get a closed signal on it (in which case the
+ * actual call would fail but the object would still be alive)
+ * while processing here.
+ */
+ connection = g_object_ref (connection_state_get_connection (state));
+
+ g_mutex_unlock (&dconf_gdbus_lock);
+
+ return g_dbus_connection_call_sync (connection,
bus_name, object_path, interface_name, method_name, parameters,
reply_type,
G_DBUS_CALL_FLAGS_NONE, -1, NULL, error);
}
diff --git a/gdbus/dconf-gdbus-thread.c b/gdbus/dconf-gdbus-thread.c
index a8985cd..f80bdb7 100644
--- a/gdbus/dconf-gdbus-thread.c
+++ b/gdbus/dconf-gdbus-thread.c
@@ -190,18 +190,32 @@ dconf_gdbus_get_bus_common (GBusType bus_type,
return g_object_ref (dconf_gdbus_get_bus_data[bus_type]);
}
+static void
+dconf_gdbus_bus_connection_closed (GDBusConnection *connection,
+ gboolean remote_peer_vanished,
+ GError *error,
+ gpointer user_data)
+{
+ GBusType bus_type = GPOINTER_TO_INT (user_data);
+
+ dconf_engine_dbus_handle_connection_closed (connection, remote_peer_vanished, error,
+ &dconf_gdbus_get_bus_lock,
+ &dconf_gdbus_get_bus_is_error[bus_type],
+ &dconf_gdbus_get_bus_data[bus_type],
+ G_CALLBACK (dconf_gdbus_bus_connection_closed),
+ user_data);
+}
+
static GDBusConnection *
dconf_gdbus_get_bus_in_worker (GBusType bus_type,
GError **error)
{
+ GDBusConnection *connection;
g_assert_cmpint (bus_type, <, G_N_ELEMENTS (dconf_gdbus_get_bus_data));
- /* We're in the worker thread and only the worker thread can ever set
- * this variable so there is no need to take a lock.
- */
+ g_mutex_lock (&dconf_gdbus_get_bus_lock);
if (dconf_gdbus_get_bus_data[bus_type] == NULL)
{
- GDBusConnection *connection;
GError *error = NULL;
gpointer result;
@@ -209,6 +223,9 @@ dconf_gdbus_get_bus_in_worker (GBusType bus_type,
if (connection)
{
+ g_signal_connect (connection, "closed",
+ G_CALLBACK (dconf_gdbus_bus_connection_closed),
+ GINT_TO_POINTER (bus_type));
g_dbus_connection_signal_subscribe (connection, NULL, "ca.desrt.dconf.Writer",
NULL, NULL, NULL, G_DBUS_SIGNAL_FLAGS_NO_MATCH_RULE,
dconf_gdbus_signal_handler, GINT_TO_POINTER (bus_type), NULL);
@@ -231,13 +248,15 @@ dconf_gdbus_get_bus_in_worker (GBusType bus_type,
* flushed all outstanding writes. The other CPU has to acquire
* the lock so it cannot have done any out-of-order reads either.
*/
- g_mutex_lock (&dconf_gdbus_get_bus_lock);
dconf_gdbus_get_bus_data[bus_type] = result;
- g_cond_broadcast (&dconf_gdbus_get_bus_cond);
- g_mutex_unlock (&dconf_gdbus_get_bus_lock);
}
- return dconf_gdbus_get_bus_common (bus_type, error);
+ connection = dconf_gdbus_get_bus_common (bus_type, error);
+
+ g_cond_broadcast (&dconf_gdbus_get_bus_cond);
+ g_mutex_unlock (&dconf_gdbus_get_bus_lock);
+
+ return connection;
}
static void
@@ -326,6 +345,8 @@ static GDBusConnection *
dconf_gdbus_get_bus_for_sync (GBusType bus_type,
GError **error)
{
+ g_autoptr(GDBusConnection) connection = NULL;
+
g_assert_cmpint (bus_type, <, G_N_ELEMENTS (dconf_gdbus_get_bus_data));
/* I'm not 100% sure we have to lock as much as we do here, but let's
@@ -344,9 +365,10 @@ dconf_gdbus_get_bus_for_sync (GBusType bus_type,
while (dconf_gdbus_get_bus_data[bus_type] == NULL)
g_cond_wait (&dconf_gdbus_get_bus_cond, &dconf_gdbus_get_bus_lock);
}
+ connection = dconf_gdbus_get_bus_common (bus_type, error);
g_mutex_unlock (&dconf_gdbus_get_bus_lock);
- return dconf_gdbus_get_bus_common (bus_type, error);
+ return g_steal_pointer (&connection);
}
GVariant *
diff --git a/gdbus/meson.build b/gdbus/meson.build
index 4fbf3ec..aad517a 100644
--- a/gdbus/meson.build
+++ b/gdbus/meson.build
@@ -1,6 +1,14 @@
+libdconf_gdbus_common_sources = files(
+ 'dconf-gdbus-common.c',
+)
+
+libdconf_gdbus_thread_sources = libdconf_gdbus_common_sources + files(
+ 'dconf-gdbus-thread.c',
+)
+
libdconf_gdbus_thread = static_library(
'dconf-gdbus-thread',
- sources: 'dconf-gdbus-thread.c',
+ sources: libdconf_gdbus_thread_sources,
include_directories: top_inc,
dependencies: libdconf_engine_dep,
c_args: dconf_c_args,
@@ -12,9 +20,13 @@ libdconf_gdbus_thread_dep = declare_dependency(
link_with: libdconf_gdbus_thread,
)
+libdconf_gdbus_filter_sources = libdconf_gdbus_common_sources + files(
+ 'dconf-gdbus-filter.c',
+)
+
libdconf_gdbus_filter = static_library(
'dconf-gdbus-filter',
- sources: 'dconf-gdbus-filter.c',
+ sources: libdconf_gdbus_filter_sources,
include_directories: top_inc,
dependencies: libdconf_engine_dep,
c_args: dconf_c_args,
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
