From 9b500297c707c36b2c378f45e4f2879da95b42a3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 12 Nov 2018 12:51:16 +0000 Subject: [PATCH 01/17] tests: Port gdbus-threading to use g_assert_*() rather than g_assert() g_assert() can be compiled out with G_DISABLE_ASSERT, which renders the test rather useless. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1515 --- gio/tests/gdbus-threading.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index b9033ead5..30c5af566 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -51,10 +51,10 @@ msg_cb_expect_success (GDBusConnection *connection, res, &error); g_assert_no_error (error); - g_assert (result != NULL); + g_assert_nonnull (result); g_variant_unref (result); - g_assert (g_thread_self () == data->thread); + g_assert_true (g_thread_self () == data->thread); g_main_loop_quit (data->thread_loop); } @@ -73,11 +73,11 @@ msg_cb_expect_error_cancelled (GDBusConnection *connection, res, &error); g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); - g_assert (!g_dbus_error_is_remote_error (error)); + g_assert_false (g_dbus_error_is_remote_error (error)); g_error_free (error); - g_assert (result == NULL); + g_assert_null (result); - g_assert (g_thread_self () == data->thread); + g_assert_true (g_thread_self () == data->thread); g_main_loop_quit (data->thread_loop); } @@ -93,7 +93,7 @@ signal_handler (GDBusConnection *connection, { DeliveryData *data = user_data; - g_assert (g_thread_self () == data->thread); + g_assert_true (g_thread_self () == data->thread); data->signal_count++; @@ -196,15 +196,15 @@ test_delivery_in_thread_func (gpointer _data) signal_handler, &data, NULL); - g_assert (subscription_id != 0); - g_assert (data.signal_count == 0); + g_assert_cmpuint (subscription_id, !=, 0); + g_assert_cmpuint (data.signal_count, ==, 0); priv_c = _g_bus_get_priv (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (priv_c != NULL); + g_assert_nonnull (priv_c); g_main_loop_run (thread_loop); - g_assert (data.signal_count == 1); + g_assert_cmpuint (data.signal_count, ==, 1); g_object_unref (priv_c); @@ -257,11 +257,11 @@ sleep_cb (GDBusProxy *proxy, res, &error); g_assert_no_error (error); - g_assert (result != NULL); + g_assert_nonnull (result); g_assert_cmpstr (g_variant_get_type_string (result), ==, "()"); g_variant_unref (result); - g_assert (data->thread == g_thread_self ()); + g_assert_true (data->thread == g_thread_self ()); g_main_loop_quit (data->thread_loop); @@ -317,7 +317,7 @@ test_sleep_in_thread_func (gpointer _data) g_printerr ("S"); //g_debug ("done invoking sync (%p)", g_thread_self ()); g_assert_no_error (error); - g_assert (result != NULL); + g_assert_nonnull (result); g_assert_cmpstr (g_variant_get_type_string (result), ==, "()"); g_variant_unref (result); } @@ -457,8 +457,8 @@ ensure_connection_works (GDBusConnection *conn) "/org/freedesktop/DBus", "org.freedesktop.DBus", "GetId", NULL, NULL, 0, -1, NULL, &error); g_assert_no_error (error); - g_assert (v != NULL); - g_assert (g_variant_is_of_type (v, G_VARIANT_TYPE ("(s)"))); + g_assert_nonnull (v); + g_assert_true (g_variant_is_of_type (v, G_VARIANT_TYPE ("(s)"))); g_variant_unref (v); } @@ -592,7 +592,7 @@ main (int argc, /* this is safe; testserver will exit once the bus goes away */ path = g_test_build_filename (G_TEST_BUILT, "gdbus-testserver", NULL); - g_assert (g_spawn_command_line_async (path, NULL)); + g_assert_true (g_spawn_command_line_async (path, NULL)); g_free (path); ensure_gdbus_testserver_up (); @@ -601,7 +601,7 @@ main (int argc, error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); - g_assert (c != NULL); + g_assert_nonnull (c); g_test_add_func ("/gdbus/delivery-in-thread", test_delivery_in_thread); g_test_add_func ("/gdbus/method-calls-in-thread", test_method_calls_in_thread); From 32e254dea26f8d632c7410536d88632454978132 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 12 Nov 2018 13:20:29 +0000 Subject: [PATCH 02/17] tests: Move refcount checks to the end of each test in gdbus-threading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These checks used to be a precondition on test_threaded_singleton(); but the earlier tests could leave the refcount of the shared connection in a bad state, and this wouldn’t be caught until later. Factor out the check, increase the iteration count to 1000 (so the check blocks for up to 1s rather than 100ms), and call it in more places. Signed-off-by: Philip Withnall https://gitlab.gnome.org/GNOME/glib/issues/1515 --- gio/tests/gdbus-threading.c | 42 ++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index 30c5af566..945d9428b 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -27,6 +27,29 @@ /* all tests rely on a global connection */ static GDBusConnection *c = NULL; +/* Check that the given @connection has only one ref, waiting to let any pending + * unrefs complete first. This is typically used on the shared connection, to + * ensure it’s in a correct state before beginning the next test. */ +static void +assert_connection_has_one_ref (GDBusConnection *connection) +{ + guint j; + + for (j = 0; j < 1000; j++) + { + guint r = g_atomic_int_get (&G_OBJECT (connection)->ref_count); + + if (r == 1) + break; + + g_debug ("refcount of %p is %u, sleeping", connection, r); + g_usleep (1000); + } + + if (j == 1000) + g_error ("connection %p had too many refs", connection); +} + /* ---------------------------------------------------------------------------------------------------- */ /* Ensure that signal and method replies are delivered in the right thread */ /* ---------------------------------------------------------------------------------------------------- */ @@ -229,6 +252,8 @@ test_delivery_in_thread (void) NULL); g_thread_join (thread); + + assert_connection_has_one_ref (c); } /* ---------------------------------------------------------------------------------------------------- */ @@ -441,6 +466,8 @@ test_method_calls_in_thread (void) if (g_test_verbose ()) g_printerr ("\n"); + + assert_connection_has_one_ref (c); } #define SLEEP_MIN_USEC 1 @@ -511,24 +538,11 @@ test_threaded_singleton (void) for (i = 0; i < n; i++) { GThread *thread; - guint j; guint unref_delay, get_delay; GDBusConnection *new_conn; /* We want to be the last ref, so let it finish setting up */ - for (j = 0; j < 100; j++) - { - guint r = (guint) g_atomic_int_get (&G_OBJECT (c)->ref_count); - - if (r == 1) - break; - - g_debug ("run %u: refcount is %u, sleeping", i, r); - g_usleep (1000); - } - - if (j == 100) - g_error ("connection had too many refs"); + assert_connection_has_one_ref (c); if (g_test_verbose () && (i % (n/50)) == 0) g_printerr ("%u%%\n", ((i * 100) / n)); From 190ef9c63060daf49a0c8a64d3a30838486306f0 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 11:09:45 +0000 Subject: [PATCH 03/17] gdbusproxy: Replace home-grown weak ref implementation with GWeakRef The fix for bgo#651133 (commit 7e0f890e38) introduced a kind of weak ref, which had to be thread-safe due to the fact that `GDBusProxy` operates in one thread but can emit signals in another. Since that commit, `GWeakRef` was added, which does the same thing. Drop the custom code in favour of it; this should be functionally equivalent, but using an RW lock rather than a basic mutex, which should reduce contention. Signed-off-by: Philip Withnall --- gio/gdbusproxy.c | 104 +++++++++++------------------------------------ 1 file changed, 23 insertions(+), 81 deletions(-) diff --git a/gio/gdbusproxy.c b/gio/gdbusproxy.c index 39eed1688..f3b02ccce 100644 --- a/gio/gdbusproxy.c +++ b/gio/gdbusproxy.c @@ -95,28 +95,19 @@ G_LOCK_DEFINE_STATIC (properties_lock); /* ---------------------------------------------------------------------------------------------------- */ -G_LOCK_DEFINE_STATIC (signal_subscription_lock); - -typedef struct -{ - volatile gint ref_count; - GDBusProxy *proxy; -} SignalSubscriptionData; - -static SignalSubscriptionData * -signal_subscription_ref (SignalSubscriptionData *data) +static GWeakRef * +weak_ref_new (GObject *object) { - g_atomic_int_inc (&data->ref_count); - return data; + GWeakRef *weak_ref = g_new0 (GWeakRef, 1); + g_weak_ref_init (weak_ref, object); + return g_steal_pointer (&weak_ref); } static void -signal_subscription_unref (SignalSubscriptionData *data) +weak_ref_free (GWeakRef *weak_ref) { - if (g_atomic_int_dec_and_test (&data->ref_count)) - { - g_slice_free (SignalSubscriptionData, data); - } + g_weak_ref_clear (weak_ref); + g_free (weak_ref); } /* ---------------------------------------------------------------------------------------------------- */ @@ -152,8 +143,6 @@ struct _GDBusProxyPrivate /* mutable, protected by properties_lock */ GDBusObject *object; - - SignalSubscriptionData *signal_subscription_data; }; enum @@ -189,22 +178,6 @@ G_DEFINE_TYPE_WITH_CODE (GDBusProxy, g_dbus_proxy, G_TYPE_OBJECT, G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE, initable_iface_init) G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, async_initable_iface_init)) -static void -g_dbus_proxy_dispose (GObject *object) -{ - GDBusProxy *proxy = G_DBUS_PROXY (object); - G_LOCK (signal_subscription_lock); - if (proxy->priv->signal_subscription_data != NULL) - { - proxy->priv->signal_subscription_data->proxy = NULL; - signal_subscription_unref (proxy->priv->signal_subscription_data); - proxy->priv->signal_subscription_data = NULL; - } - G_UNLOCK (signal_subscription_lock); - - G_OBJECT_CLASS (g_dbus_proxy_parent_class)->dispose (object); -} - static void g_dbus_proxy_finalize (GObject *object) { @@ -346,7 +319,6 @@ g_dbus_proxy_class_init (GDBusProxyClass *klass) { GObjectClass *gobject_class = G_OBJECT_CLASS (klass); - gobject_class->dispose = g_dbus_proxy_dispose; gobject_class->finalize = g_dbus_proxy_finalize; gobject_class->set_property = g_dbus_proxy_set_property; gobject_class->get_property = g_dbus_proxy_get_property; @@ -638,9 +610,6 @@ static void g_dbus_proxy_init (GDBusProxy *proxy) { proxy->priv = g_dbus_proxy_get_instance_private (proxy); - proxy->priv->signal_subscription_data = g_slice_new0 (SignalSubscriptionData); - proxy->priv->signal_subscription_data->ref_count = 1; - proxy->priv->signal_subscription_data->proxy = proxy; proxy->priv->properties = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, @@ -868,21 +837,12 @@ on_signal_received (GDBusConnection *connection, GVariant *parameters, gpointer user_data) { - SignalSubscriptionData *data = user_data; + GWeakRef *proxy_weak = user_data; GDBusProxy *proxy; - G_LOCK (signal_subscription_lock); - proxy = data->proxy; + proxy = G_DBUS_PROXY (g_weak_ref_get (proxy_weak)); if (proxy == NULL) - { - G_UNLOCK (signal_subscription_lock); - return; - } - else - { - g_object_ref (proxy); - G_UNLOCK (signal_subscription_lock); - } + return; if (!proxy->priv->initialized) goto out; @@ -1038,7 +998,7 @@ on_properties_changed (GDBusConnection *connection, GVariant *parameters, gpointer user_data) { - SignalSubscriptionData *data = user_data; + GWeakRef *proxy_weak = user_data; gboolean emit_g_signal = FALSE; GDBusProxy *proxy; const gchar *interface_name_for_signal; @@ -1052,18 +1012,9 @@ on_properties_changed (GDBusConnection *connection, changed_properties = NULL; invalidated_properties = NULL; - G_LOCK (signal_subscription_lock); - proxy = data->proxy; + proxy = G_DBUS_PROXY (g_weak_ref_get (proxy_weak)); if (proxy == NULL) - { - G_UNLOCK (signal_subscription_lock); - goto out; - } - else - { - g_object_ref (proxy); - G_UNLOCK (signal_subscription_lock); - } + return; if (!proxy->priv->initialized) goto out; @@ -1289,23 +1240,14 @@ on_name_owner_changed (GDBusConnection *connection, GVariant *parameters, gpointer user_data) { - SignalSubscriptionData *data = user_data; + GWeakRef *proxy_weak = user_data; GDBusProxy *proxy; const gchar *old_owner; const gchar *new_owner; - G_LOCK (signal_subscription_lock); - proxy = data->proxy; + proxy = G_DBUS_PROXY (g_weak_ref_get (proxy_weak)); if (proxy == NULL) - { - G_UNLOCK (signal_subscription_lock); - goto out; - } - else - { - g_object_ref (proxy); - G_UNLOCK (signal_subscription_lock); - } + return; /* if we are already trying to load properties, cancel that */ if (proxy->priv->get_all_cancellable != NULL) @@ -1762,8 +1704,8 @@ async_initable_init_first (GAsyncInitable *initable) proxy->priv->interface_name, G_DBUS_SIGNAL_FLAGS_NONE, on_properties_changed, - signal_subscription_ref (proxy->priv->signal_subscription_data), - (GDestroyNotify) signal_subscription_unref); + weak_ref_new (G_OBJECT (proxy)), + (GDestroyNotify) weak_ref_free); } if (!(proxy->priv->flags & G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS)) @@ -1778,8 +1720,8 @@ async_initable_init_first (GAsyncInitable *initable) NULL, /* arg0 */ G_DBUS_SIGNAL_FLAGS_NONE, on_signal_received, - signal_subscription_ref (proxy->priv->signal_subscription_data), - (GDestroyNotify) signal_subscription_unref); + weak_ref_new (G_OBJECT (proxy)), + (GDestroyNotify) weak_ref_free); } if (proxy->priv->name != NULL && @@ -1794,8 +1736,8 @@ async_initable_init_first (GAsyncInitable *initable) proxy->priv->name, /* arg0 */ G_DBUS_SIGNAL_FLAGS_NONE, on_name_owner_changed, - signal_subscription_ref (proxy->priv->signal_subscription_data), - (GDestroyNotify) signal_subscription_unref); + weak_ref_new (G_OBJECT (proxy)), + (GDestroyNotify) weak_ref_free); } } From e5d4802bb9c259144adaacab3cf49883c40421a5 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 11:23:50 +0000 Subject: [PATCH 04/17] gdbusproxy: Tidy up some memory management code Use `g_clear_object()` to tidy things up. This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gdbusproxy.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/gio/gdbusproxy.c b/gio/gdbusproxy.c index f3b02ccce..22ccc830f 100644 --- a/gio/gdbusproxy.c +++ b/gio/gdbusproxy.c @@ -889,8 +889,7 @@ on_signal_received (GDBusConnection *connection, parameters); out: - if (proxy != NULL) - g_object_unref (proxy); + g_clear_object (&proxy); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1101,11 +1100,9 @@ on_properties_changed (GDBusConnection *connection, } out: - if (changed_properties != NULL) - g_variant_unref (changed_properties); + g_clear_pointer (&changed_properties, g_variant_unref); g_free (invalidated_properties); - if (proxy != NULL) - g_object_unref (proxy); + g_clear_object (&proxy); } /* ---------------------------------------------------------------------------------------------------- */ @@ -1357,8 +1354,7 @@ on_name_owner_changed (GDBusConnection *connection, } out: - if (proxy != NULL) - g_object_unref (proxy); + g_clear_object (&proxy); } /* ---------------------------------------------------------------------------------------------------- */ From 0d3cae85cd482708c8d79da55b764a025fdcd6d3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 11:47:47 +0000 Subject: [PATCH 05/17] gdbusproxy: Simplify a pointer theft This introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gdbusproxy.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gio/gdbusproxy.c b/gio/gdbusproxy.c index 22ccc830f..4c682978d 100644 --- a/gio/gdbusproxy.c +++ b/gio/gdbusproxy.c @@ -1206,8 +1206,7 @@ on_name_owner_changed_get_all_cb (GDBusConnection *connection, { G_LOCK (properties_lock); g_free (data->proxy->priv->name_owner); - data->proxy->priv->name_owner = data->name_owner; - data->name_owner = NULL; /* to avoid an extra copy, we steal the string */ + data->proxy->priv->name_owner = g_steal_pointer (&data->name_owner); g_hash_table_remove_all (data->proxy->priv->properties); G_UNLOCK (properties_lock); if (result != NULL) From 381d29f7d511936588939206c5f89a22fcd9ab23 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 12:40:30 +0000 Subject: [PATCH 06/17] gdbusconnection: Drop an unnecessary GMainContext reference `CallDestroyNotifyData` never uses that `GMainContext`, and holding a ref to it could cause reference count cycles if the `GMainContext` is no longer being iterated. Signed-off-by: Philip Withnall Helps: #1515 --- gio/gdbusconnection.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 67496be5b..5f5fad0f1 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -222,7 +222,6 @@ typedef struct { GDestroyNotify callback; gpointer user_data; - GMainContext *context; } CallDestroyNotifyData; static gboolean @@ -236,8 +235,6 @@ call_destroy_notify_data_in_idle (gpointer user_data) static void call_destroy_notify_data_free (CallDestroyNotifyData *data) { - if (data->context != NULL) - g_main_context_unref (data->context); g_free (data); } @@ -263,9 +260,6 @@ call_destroy_notify (GMainContext *context, data = g_new0 (CallDestroyNotifyData, 1); data->callback = callback; data->user_data = user_data; - data->context = context; - if (data->context != NULL) - g_main_context_ref (data->context); idle_source = g_idle_source_new (); g_source_set_priority (idle_source, G_PRIORITY_DEFAULT); @@ -274,7 +268,7 @@ call_destroy_notify (GMainContext *context, data, (GDestroyNotify) call_destroy_notify_data_free); g_source_set_name (idle_source, "[gio] call_destroy_notify_data_in_idle"); - g_source_attach (idle_source, data->context); + g_source_attach (idle_source, context); g_source_unref (idle_source); out: From 87c18ad767bf08ef4c354f2c2335f1bb81badf40 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 13:20:41 +0000 Subject: [PATCH 07/17] gdbusconnection: Simplify some control flow This removes an unhelpful `goto`. It introduces no functional changes. Signed-off-by: Philip Withnall --- gio/gdbusconnection.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 5f5fad0f1..b58b1b4a3 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -255,7 +255,7 @@ call_destroy_notify (GMainContext *context, CallDestroyNotifyData *data; if (callback == NULL) - goto out; + return; data = g_new0 (CallDestroyNotifyData, 1); data->callback = callback; @@ -270,9 +270,6 @@ call_destroy_notify (GMainContext *context, g_source_set_name (idle_source, "[gio] call_destroy_notify_data_in_idle"); g_source_attach (idle_source, context); g_source_unref (idle_source); - - out: - ; } /* ---------------------------------------------------------------------------------------------------- */ From 07093f6acb42743c8e7ba8efc4f6873a7d2c794b Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 20 Feb 2020 11:49:12 +0000 Subject: [PATCH 08/17] tests: Take explicit connection and context when ensuring testserver up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This introduces no functional changes, but makes the code a little more explicit about which connection and main context it’s operating on. Signed-off-by: Philip Withnall Helps: #1515 --- gio/tests/gdbus-connection-loss.c | 4 ++-- gio/tests/gdbus-tests.c | 28 ++++++++++++++-------------- gio/tests/gdbus-tests.h | 3 ++- gio/tests/gdbus-threading.c | 4 ++-- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/gio/tests/gdbus-connection-loss.c b/gio/tests/gdbus-connection-loss.c index 8f7023f3f..a34a9923c 100644 --- a/gio/tests/gdbus-connection-loss.c +++ b/gio/tests/gdbus-connection-loss.c @@ -124,14 +124,14 @@ main (int argc, g_assert (g_spawn_command_line_async (path, NULL)); g_free (path); - ensure_gdbus_testserver_up (); - /* Create the connection in the main thread */ error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); g_assert (c != NULL); + ensure_gdbus_testserver_up (c, NULL); + g_test_add_func ("/gdbus/connection-loss", test_connection_loss); ret = g_test_run(); diff --git a/gio/tests/gdbus-tests.c b/gio/tests/gdbus-tests.c index 35e379bd2..66f16e02f 100644 --- a/gio/tests/gdbus-tests.c +++ b/gio/tests/gdbus-tests.c @@ -87,20 +87,15 @@ _give_up (gpointer data) } void -ensure_gdbus_testserver_up (void) +ensure_gdbus_testserver_up (GDBusConnection *connection, + GMainContext *context) { - guint id; gchar *name_owner; - GDBusConnection *connection; + GSource *timeout_source = NULL; GDBusProxy *proxy; GError *error = NULL; - connection = g_bus_get_sync (G_BUS_TYPE_SESSION, - NULL, - &error); - - g_assert_no_error (error); - error = NULL; + g_main_context_push_thread_default (context); proxy = g_dbus_proxy_new_sync (connection, G_DBUS_PROXY_FLAGS_NONE, @@ -112,8 +107,11 @@ ensure_gdbus_testserver_up (void) &error); g_assert_no_error (error); - id = g_timeout_add_seconds (60, _give_up, - "waited more than ~ 60s for gdbus-testserver to take its bus name"); + timeout_source = g_timeout_source_new_seconds (60); + g_source_set_callback (timeout_source, _give_up, + "waited more than ~ 60s for gdbus-testserver to take its bus name", + NULL); + g_source_attach (timeout_source, context); while (TRUE) { @@ -122,13 +120,15 @@ ensure_gdbus_testserver_up (void) if (name_owner != NULL) break; - g_main_context_iteration (NULL, TRUE); + g_main_context_iteration (context, TRUE); } - g_source_remove (id); + g_source_destroy (timeout_source); + g_source_unref (timeout_source); g_free (name_owner); g_object_unref (proxy); - g_object_unref (connection); + + g_main_context_pop_thread_default (context); } /* ---------------------------------------------------------------------------------------------------- */ diff --git a/gio/tests/gdbus-tests.h b/gio/tests/gdbus-tests.h index 00cda3713..eaef23480 100644 --- a/gio/tests/gdbus-tests.h +++ b/gio/tests/gdbus-tests.h @@ -114,7 +114,8 @@ GDBusConnection *_g_bus_get_priv (GBusType bus_type, GCancellable *cancellable, GError **error); -void ensure_gdbus_testserver_up (void); +void ensure_gdbus_testserver_up (GDBusConnection *connection, + GMainContext *context); G_END_DECLS diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index 945d9428b..fe95532cb 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -609,14 +609,14 @@ main (int argc, g_assert_true (g_spawn_command_line_async (path, NULL)); g_free (path); - ensure_gdbus_testserver_up (); - /* Create the connection in the main thread */ error = NULL; c = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error); g_assert_no_error (error); g_assert_nonnull (c); + ensure_gdbus_testserver_up (c, NULL); + g_test_add_func ("/gdbus/delivery-in-thread", test_delivery_in_thread); g_test_add_func ("/gdbus/method-calls-in-thread", test_method_calls_in_thread); g_test_add_func ("/gdbus/threaded-singleton", test_threaded_singleton); From 28ede9b4866fd7d295cdb437946f78a13f71b582 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 12:18:29 +0000 Subject: [PATCH 09/17] tests: Add timeout to assert_connection_has_one_ref() Iterate the given `context` while waiting, rather than sleeping. This ensures that if the errant `GDBusConnection` ref is held by some pending callback in the given `context`, it will actually be released. Typically `context` is going to be the global default main context. Signed-off-by: Philip Withnall Helps: #1515 --- gio/tests/gdbus-threading.c | 50 +++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index fe95532cb..a1b562484 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -27,26 +27,50 @@ /* all tests rely on a global connection */ static GDBusConnection *c = NULL; +typedef struct +{ + GMainContext *context; + gboolean timed_out; +} TimeoutData; + +static gboolean +timeout_cb (gpointer user_data) +{ + TimeoutData *data = user_data; + + data->timed_out = TRUE; + g_main_context_wakeup (data->context); + + return G_SOURCE_REMOVE; +} + /* Check that the given @connection has only one ref, waiting to let any pending * unrefs complete first. This is typically used on the shared connection, to * ensure it’s in a correct state before beginning the next test. */ static void -assert_connection_has_one_ref (GDBusConnection *connection) +assert_connection_has_one_ref (GDBusConnection *connection, + GMainContext *context) { - guint j; + GSource *timeout_source = NULL; + TimeoutData data = { context, FALSE }; - for (j = 0; j < 1000; j++) - { - guint r = g_atomic_int_get (&G_OBJECT (connection)->ref_count); + if (g_atomic_int_get (&G_OBJECT (connection)->ref_count) == 1) + return; - if (r == 1) - break; + timeout_source = g_timeout_source_new_seconds (1); + g_source_set_callback (timeout_source, timeout_cb, &data, NULL); + g_source_attach (timeout_source, context); - g_debug ("refcount of %p is %u, sleeping", connection, r); - g_usleep (1000); + while (g_atomic_int_get (&G_OBJECT (connection)->ref_count) != 1 && !data.timed_out) + { + g_debug ("refcount of %p is not right, sleeping", connection); + g_main_context_iteration (NULL, TRUE); } - if (j == 1000) + g_source_destroy (timeout_source); + g_source_unref (timeout_source); + + if (g_atomic_int_get (&G_OBJECT (connection)->ref_count) != 1) g_error ("connection %p had too many refs", connection); } @@ -253,7 +277,7 @@ test_delivery_in_thread (void) g_thread_join (thread); - assert_connection_has_one_ref (c); + assert_connection_has_one_ref (c, NULL); } /* ---------------------------------------------------------------------------------------------------- */ @@ -467,7 +491,7 @@ test_method_calls_in_thread (void) if (g_test_verbose ()) g_printerr ("\n"); - assert_connection_has_one_ref (c); + assert_connection_has_one_ref (c, NULL); } #define SLEEP_MIN_USEC 1 @@ -542,7 +566,7 @@ test_threaded_singleton (void) GDBusConnection *new_conn; /* We want to be the last ref, so let it finish setting up */ - assert_connection_has_one_ref (c); + assert_connection_has_one_ref (c, NULL); if (g_test_verbose () && (i % (n/50)) == 0) g_printerr ("%u%%\n", ((i * 100) / n)); From 457b4bb2239e40e66e002d89c7e358a89997f660 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 12:07:53 +0000 Subject: [PATCH 10/17] tests: Wait until unwatching the gdbus-testserver name has completed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the code in `ensure_gdbus_testserver_up()` created a proxy object and watched its `name-owner` to see when the `com.example.TestService` name appeared. This ended up subscribing to three signals (one of them for name ownership, and two unused for properties of the proxy), and was racy. In particular, the `name-owner` property could be set before all D-Bus messages had been processed — it could have been derived from getting the owner of the name, for example. This left unprocessed messages hanging around in the `context`, but that context was never iterated again, which essentially leaked the references held by those messages. That included a reference to the `GDBusConnection`. The first part of the fix is to simplify the code to use `g_bus_watch_name_on_connection()`, so there’s only one signal subscription to worry about. The second part of the fix is to use the `GDestroyNotify` callback for the watch data to be notified of when all D-Bus traffic has been processed and the signal unsubscription is complete. At this point, it’s guaranteed that there are no idle callbacks pending in the `GMainContext`, since the `GDestroyNotify` callback is the last one invoked on the `GMainContext`. Essentially, this commit uses the `GDestroyNotify` callback as a synchronisation message between the D-Bus worker thread and the thread calling `ensure_gdbus_testserver_up()`. Signed-off-by: Philip Withnall Fixes: #1515 --- gio/tests/gdbus-tests.c | 65 ++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/gio/tests/gdbus-tests.c b/gio/tests/gdbus-tests.c index 66f16e02f..bed0d24f7 100644 --- a/gio/tests/gdbus-tests.c +++ b/gio/tests/gdbus-tests.c @@ -86,26 +86,52 @@ _give_up (gpointer data) g_return_val_if_reached (TRUE); } +typedef struct +{ + GMainContext *context; + gboolean name_appeared; + gboolean unwatch_complete; +} WatchData; + +static void +name_appeared_cb (GDBusConnection *connection, + const gchar *name, + const gchar *name_owner, + gpointer user_data) +{ + WatchData *data = user_data; + + g_assert (name_owner != NULL); + data->name_appeared = TRUE; + g_main_context_wakeup (data->context); +} + +static void +watch_free_cb (gpointer user_data) +{ + WatchData *data = user_data; + + data->unwatch_complete = TRUE; + g_main_context_wakeup (data->context); +} + void ensure_gdbus_testserver_up (GDBusConnection *connection, GMainContext *context) { - gchar *name_owner; GSource *timeout_source = NULL; - GDBusProxy *proxy; - GError *error = NULL; + guint watch_id; + WatchData data = { context, FALSE, FALSE }; g_main_context_push_thread_default (context); - proxy = g_dbus_proxy_new_sync (connection, - G_DBUS_PROXY_FLAGS_NONE, - NULL, /* GDBusInterfaceInfo */ - "com.example.TestService", /* name */ - "/com/example/TestObject", /* object path */ - "com.example.Frob", /* interface */ - NULL, /* GCancellable */ - &error); - g_assert_no_error (error); + watch_id = g_bus_watch_name_on_connection (connection, + "com.example.TestService", + G_BUS_NAME_WATCHER_FLAGS_NONE, + name_appeared_cb, + NULL, + &data, + watch_free_cb); timeout_source = g_timeout_source_new_seconds (60); g_source_set_callback (timeout_source, _give_up, @@ -113,20 +139,17 @@ ensure_gdbus_testserver_up (GDBusConnection *connection, NULL); g_source_attach (timeout_source, context); - while (TRUE) - { - name_owner = g_dbus_proxy_get_name_owner (proxy); + while (!data.name_appeared) + g_main_context_iteration (context, TRUE); - if (name_owner != NULL) - break; + g_bus_unwatch_name (watch_id); + watch_id = 0; - g_main_context_iteration (context, TRUE); - } + while (!data.unwatch_complete) + g_main_context_iteration (context, TRUE); g_source_destroy (timeout_source); g_source_unref (timeout_source); - g_free (name_owner); - g_object_unref (proxy); g_main_context_pop_thread_default (context); } From 997734877629f9a4ba7761850d9ec2cce63ec49d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 12:08:57 +0000 Subject: [PATCH 11/17] tests: Wait until unsubscribing from a signal has completed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As with the previous commit, don’t stop iterating the `context` in `test_delivery_in_thread_func()` until the unsubscription from a signal is complete, and hence there’s a guarantee that no callbacks are pending in the `thread_context`. This commit uses the `GDestroyNotify` for `g_dbus_connection_signal_subscribe()` as a synchronisation message from the D-Bus worker thread to the `test_delivery_in_thread_func()` thread to notify of signal unsubscription. Signed-off-by: Philip Withnall Fixes: #1515 --- gio/tests/gdbus-threading.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index a1b562484..53f4ee0de 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -82,6 +82,7 @@ typedef struct { GThread *thread; GMainLoop *thread_loop; guint signal_count; + gboolean unsubscribe_complete; } DeliveryData; static void @@ -147,6 +148,18 @@ signal_handler (GDBusConnection *connection, g_main_loop_quit (data->thread_loop); } +static void +signal_data_free_cb (gpointer user_data) +{ + DeliveryData *data = user_data; + + g_assert_true (g_thread_self () == data->thread); + + data->unsubscribe_complete = TRUE; + + g_main_loop_quit (data->thread_loop); +} + static gpointer test_delivery_in_thread_func (gpointer _data) { @@ -167,6 +180,7 @@ test_delivery_in_thread_func (gpointer _data) data.thread = g_thread_self (); data.thread_loop = thread_loop; data.signal_count = 0; + data.unsubscribe_complete = FALSE; /* ---------------------------------------------------------------------------------------------------- */ @@ -242,7 +256,7 @@ test_delivery_in_thread_func (gpointer _data) G_DBUS_SIGNAL_FLAGS_NONE, signal_handler, &data, - NULL); + signal_data_free_cb); g_assert_cmpuint (subscription_id, !=, 0); g_assert_cmpuint (data.signal_count, ==, 0); @@ -256,6 +270,10 @@ test_delivery_in_thread_func (gpointer _data) g_object_unref (priv_c); g_dbus_connection_signal_unsubscribe (c, subscription_id); + subscription_id = 0; + + g_main_loop_run (thread_loop); + g_assert_true (data.unsubscribe_complete); /* ---------------------------------------------------------------------------------------------------- */ From b0c58e5768432c60fb35995dc6f83526991e5500 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 14:12:52 +0000 Subject: [PATCH 12/17] tests: Use GMainContext instead of GMainLoop in gdbus-threading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is equivalent, but makes the loop exit conditions a little clearer, since they’re actually in a `while` statement, rather than being a `g_main_loop_quit()` call in a callback somewhere else in the file. Signed-off-by: Philip Withnall Helps: #1515 --- gio/tests/gdbus-threading.c | 103 +++++++++++++++++------------------- 1 file changed, 49 insertions(+), 54 deletions(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index 53f4ee0de..68564675a 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -80,54 +80,24 @@ assert_connection_has_one_ref (GDBusConnection *connection, typedef struct { GThread *thread; - GMainLoop *thread_loop; + GMainContext *context; guint signal_count; gboolean unsubscribe_complete; + GAsyncResult *async_result; } DeliveryData; static void -msg_cb_expect_success (GDBusConnection *connection, - GAsyncResult *res, - gpointer user_data) +async_result_cb (GDBusConnection *connection, + GAsyncResult *res, + gpointer user_data) { DeliveryData *data = user_data; - GError *error; - GVariant *result; - - error = NULL; - result = g_dbus_connection_call_finish (connection, - res, - &error); - g_assert_no_error (error); - g_assert_nonnull (result); - g_variant_unref (result); - g_assert_true (g_thread_self () == data->thread); - - g_main_loop_quit (data->thread_loop); -} - -static void -msg_cb_expect_error_cancelled (GDBusConnection *connection, - GAsyncResult *res, - gpointer user_data) -{ - DeliveryData *data = user_data; - GError *error; - GVariant *result; - - error = NULL; - result = g_dbus_connection_call_finish (connection, - res, - &error); - g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); - g_assert_false (g_dbus_error_is_remote_error (error)); - g_error_free (error); - g_assert_null (result); + data->async_result = g_object_ref (res); g_assert_true (g_thread_self () == data->thread); - g_main_loop_quit (data->thread_loop); + g_main_context_wakeup (data->context); } static void @@ -145,7 +115,7 @@ signal_handler (GDBusConnection *connection, data->signal_count++; - g_main_loop_quit (data->thread_loop); + g_main_context_wakeup (data->context); } static void @@ -157,30 +127,28 @@ signal_data_free_cb (gpointer user_data) data->unsubscribe_complete = TRUE; - g_main_loop_quit (data->thread_loop); + g_main_context_wakeup (data->context); } static gpointer test_delivery_in_thread_func (gpointer _data) { - GMainLoop *thread_loop; GMainContext *thread_context; DeliveryData data; GCancellable *ca; guint subscription_id; GDBusConnection *priv_c; - GError *error; - - error = NULL; + GError *error = NULL; + GVariant *result_variant = NULL; thread_context = g_main_context_new (); - thread_loop = g_main_loop_new (thread_context, FALSE); g_main_context_push_thread_default (thread_context); data.thread = g_thread_self (); - data.thread_loop = thread_loop; + data.context = thread_context; data.signal_count = 0; data.unsubscribe_complete = FALSE; + data.async_result = NULL; /* ---------------------------------------------------------------------------------------------------- */ @@ -196,9 +164,16 @@ test_delivery_in_thread_func (gpointer _data) G_DBUS_CALL_FLAGS_NONE, -1, NULL, - (GAsyncReadyCallback) msg_cb_expect_success, + (GAsyncReadyCallback) async_result_cb, &data); - g_main_loop_run (thread_loop); + while (data.async_result == NULL) + g_main_context_iteration (thread_context, TRUE); + + result_variant = g_dbus_connection_call_finish (c, data.async_result, &error); + g_assert_no_error (error); + g_assert_nonnull (result_variant); + g_clear_pointer (&result_variant, g_variant_unref); + g_clear_object (&data.async_result); /* * Check that we never actually send a message if the GCancellable @@ -216,9 +191,18 @@ test_delivery_in_thread_func (gpointer _data) G_DBUS_CALL_FLAGS_NONE, -1, ca, - (GAsyncReadyCallback) msg_cb_expect_error_cancelled, + (GAsyncReadyCallback) async_result_cb, &data); - g_main_loop_run (thread_loop); + while (data.async_result == NULL) + g_main_context_iteration (thread_context, TRUE); + + result_variant = g_dbus_connection_call_finish (c, data.async_result, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); + g_assert_false (g_dbus_error_is_remote_error (error)); + g_clear_error (&error); + g_assert_null (result_variant); + g_clear_object (&data.async_result); + g_object_unref (ca); /* @@ -234,10 +218,20 @@ test_delivery_in_thread_func (gpointer _data) G_DBUS_CALL_FLAGS_NONE, -1, ca, - (GAsyncReadyCallback) msg_cb_expect_error_cancelled, + (GAsyncReadyCallback) async_result_cb, &data); g_cancellable_cancel (ca); - g_main_loop_run (thread_loop); + + while (data.async_result == NULL) + g_main_context_iteration (thread_context, TRUE); + + result_variant = g_dbus_connection_call_finish (c, data.async_result, &error); + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_CANCELLED); + g_assert_false (g_dbus_error_is_remote_error (error)); + g_clear_error (&error); + g_assert_null (result_variant); + g_clear_object (&data.async_result); + g_object_unref (ca); /* @@ -264,7 +258,8 @@ test_delivery_in_thread_func (gpointer _data) g_assert_no_error (error); g_assert_nonnull (priv_c); - g_main_loop_run (thread_loop); + while (data.signal_count < 1) + g_main_context_iteration (thread_context, TRUE); g_assert_cmpuint (data.signal_count, ==, 1); g_object_unref (priv_c); @@ -272,13 +267,13 @@ test_delivery_in_thread_func (gpointer _data) g_dbus_connection_signal_unsubscribe (c, subscription_id); subscription_id = 0; - g_main_loop_run (thread_loop); + while (!data.unsubscribe_complete) + g_main_context_iteration (thread_context, TRUE); g_assert_true (data.unsubscribe_complete); /* ---------------------------------------------------------------------------------------------------- */ g_main_context_pop_thread_default (thread_context); - g_main_loop_unref (thread_loop); g_main_context_unref (thread_context); return NULL; From b4dbb8e0f3ee731fc2b5e4b4a979868af4fd17aa Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 14:14:57 +0000 Subject: [PATCH 13/17] tests: Use TestSignal rather than NameOwnerChanged to test signals When testing that signals are delivered to the correct thread, and are delivered the correct number of times, call `EmitSignal()` on the `gdbus-testserver` to trigger a signal emission, and listen for that. Previously, the code listened for `NameOwnerChanged` and connected to the bus again to trigger emission of that. The problem with that is that other things happening on the bus (for example, an old `gdbus-testserver` instance disconnecting) can cause `NameOwnerChanged` signal emissions. Sometimes, the `gdbus-threading` test was failing the `signal_count == 1` assertion due to receiving more than one `NameOwnerChanged` emission. Signed-off-by: Philip Withnall Helps: #1515 --- gio/tests/gdbus-threading.c | 39 ++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/gio/tests/gdbus-threading.c b/gio/tests/gdbus-threading.c index 68564675a..e1a37f9a9 100644 --- a/gio/tests/gdbus-threading.c +++ b/gio/tests/gdbus-threading.c @@ -137,7 +137,6 @@ test_delivery_in_thread_func (gpointer _data) DeliveryData data; GCancellable *ca; guint subscription_id; - GDBusConnection *priv_c; GError *error = NULL; GVariant *result_variant = NULL; @@ -237,15 +236,14 @@ test_delivery_in_thread_func (gpointer _data) /* * Check that signals are delivered to the correct thread. * - * First we subscribe to the signal, then we create a a private - * connection. This should cause a NameOwnerChanged message from - * the message bus. + * First we subscribe to the signal, then we call EmitSignal(). This should + * cause a TestSignal emission from the testserver. */ subscription_id = g_dbus_connection_signal_subscribe (c, - "org.freedesktop.DBus", /* sender */ - "org.freedesktop.DBus", /* interface */ - "NameOwnerChanged", /* member */ - "/org/freedesktop/DBus", /* path */ + "com.example.TestService", /* sender */ + "com.example.Frob", /* interface */ + "TestSignal", /* member */ + "/com/example/TestObject", /* path */ NULL, G_DBUS_SIGNAL_FLAGS_NONE, signal_handler, @@ -254,16 +252,29 @@ test_delivery_in_thread_func (gpointer _data) g_assert_cmpuint (subscription_id, !=, 0); g_assert_cmpuint (data.signal_count, ==, 0); - priv_c = _g_bus_get_priv (G_BUS_TYPE_SESSION, NULL, &error); + g_dbus_connection_call (c, + "com.example.TestService", /* bus_name */ + "/com/example/TestObject", /* object path */ + "com.example.Frob", /* interface name */ + "EmitSignal", /* method name */ + g_variant_new_parsed ("('hello', @o '/com/example/TestObject')"), + NULL, + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + (GAsyncReadyCallback) async_result_cb, + &data); + while (data.async_result == NULL || data.signal_count < 1) + g_main_context_iteration (thread_context, TRUE); + + result_variant = g_dbus_connection_call_finish (c, data.async_result, &error); g_assert_no_error (error); - g_assert_nonnull (priv_c); + g_assert_nonnull (result_variant); + g_clear_pointer (&result_variant, g_variant_unref); + g_clear_object (&data.async_result); - while (data.signal_count < 1) - g_main_context_iteration (thread_context, TRUE); g_assert_cmpuint (data.signal_count, ==, 1); - g_object_unref (priv_c); - g_dbus_connection_signal_unsubscribe (c, subscription_id); subscription_id = 0; From 62ecc923c9cea3d64d5bdb9f97efdc92e999dbc3 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 12:19:00 +0000 Subject: [PATCH 14/17] tests: Mark gdbus-threading as non-flaky any more See previous commits and #1515. Signed-off-by: Philip Withnall Fixes: #1515 --- gio/tests/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/tests/meson.build b/gio/tests/meson.build index 22028cc74..eb60323cf 100644 --- a/gio/tests/meson.build +++ b/gio/tests/meson.build @@ -303,7 +303,7 @@ if host_machine.system() != 'windows' }, 'gdbus-threading' : { 'extra_sources' : extra_sources, - 'suite' : ['slow', 'flaky'], + 'suite' : ['slow'], }, 'gmenumodel' : { 'extra_sources' : extra_sources, From 3d3f5d1d7f08db597466655e0bcfd864c17d44b9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 21 Feb 2020 15:12:08 +0000 Subject: [PATCH 15/17] gdbusconnection: Document main context iteration for unsubscriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a note to the documentation of `g_dbus_connection_signal_unsubscribe()`, `g_bus_unwatch_name()` and `g_bus_unown_name()` warning about the need to continue iterating the caller’s thread-default `GMainContext` until the unsubscribe/unwatch/unown operation is complete. See the previous few commits and #1515 for an idea of the insidious bugs that can be caused by not iterating the `GMainContext` until everything’s synchronised. Signed-off-by: Philip Withnall --- gio/gdbusconnection.c | 8 ++++++++ gio/gdbusnameowning.c | 7 +++++++ gio/gdbusnamewatching.c | 7 +++++++ 3 files changed, 22 insertions(+) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index b58b1b4a3..1a4dae3bd 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -3694,6 +3694,14 @@ unsubscribe_id_internal (GDBusConnection *connection, * * Unsubscribes from signals. * + * Note that there may still be D-Bus traffic to process (relating to this + * signal subscription) in the current thread-default #GMainContext after this + * function has returned. You should continue to iterate the #GMainContext + * until the #GDestroyNotify function passed to + * g_dbus_connection_signal_subscribe() is called, in order to avoid memory + * leaks through callbacks queued on the #GMainContext after it’s stopped being + * iterated. + * * Since: 2.26 */ void diff --git a/gio/gdbusnameowning.c b/gio/gdbusnameowning.c index 839a88cfb..9f2a3039e 100644 --- a/gio/gdbusnameowning.c +++ b/gio/gdbusnameowning.c @@ -877,6 +877,13 @@ g_bus_own_name_on_connection_with_closures (GDBusConnection *connection, * * Stops owning a name. * + * Note that there may still be D-Bus traffic to process (relating to owning + * and unowning the name) in the current thread-default #GMainContext after + * this function has returned. You should continue to iterate the #GMainContext + * until the #GDestroyNotify function passed to g_bus_own_name() is called, in + * order to avoid memory leaks through callbacks queued on the #GMainContext + * after it’s stopped being iterated. + * * Since: 2.26 */ void diff --git a/gio/gdbusnamewatching.c b/gio/gdbusnamewatching.c index c103bb484..bc2a9119e 100644 --- a/gio/gdbusnamewatching.c +++ b/gio/gdbusnamewatching.c @@ -858,6 +858,13 @@ guint g_bus_watch_name_on_connection_with_closures ( * * Stops watching a name. * + * Note that there may still be D-Bus traffic to process (relating to watching + * and unwatching the name) in the current thread-default #GMainContext after + * this function has returned. You should continue to iterate the #GMainContext + * until the #GDestroyNotify function passed to g_bus_watch_name() is called, in + * order to avoid memory leaks through callbacks queued on the #GMainContext + * after it’s stopped being iterated. + * * Since: 2.26 */ void From 2c97fb653d016d99eabc37d387a1d7806d02ca22 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 24 Feb 2020 08:33:02 +0000 Subject: [PATCH 16/17] tests: Skip g-file-info-filesystem-readonly test if bindfs fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bindfs is part of the setup process, so if it fails (as can happen if the `fuse` kernel module has not been loaded — not much we can do about that) then skip the test. Signed-off-by: Philip Withnall --- gio/tests/g-file-info-filesystem-readonly.c | 24 ++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/gio/tests/g-file-info-filesystem-readonly.c b/gio/tests/g-file-info-filesystem-readonly.c index 60937b143..16fa83e33 100644 --- a/gio/tests/g-file-info-filesystem-readonly.c +++ b/gio/tests/g-file-info-filesystem-readonly.c @@ -25,7 +25,7 @@ #include #include -static void +static gboolean run (GError **error, const gchar *argv0, ...) @@ -34,6 +34,8 @@ run (GError **error, const gchar *arg; va_list ap; GSubprocess *subprocess; + gchar *command_line = NULL; + gboolean success; args = g_ptr_array_new (); @@ -44,14 +46,20 @@ run (GError **error, g_ptr_array_add (args, NULL); va_end (ap); + command_line = g_strjoinv (" ", (gchar **) args->pdata); + g_test_message ("Running command `%s`", command_line); + g_free (command_line); + subprocess = g_subprocess_newv ((const gchar * const *) args->pdata, G_SUBPROCESS_FLAGS_NONE, error); g_ptr_array_free (args, TRUE); if (subprocess == NULL) - return; + return FALSE; - g_subprocess_wait_check (subprocess, NULL, error); + success = g_subprocess_wait_check (subprocess, NULL, error); g_object_unref (subprocess); + + return success; } static void @@ -135,8 +143,14 @@ test_filesystem_readonly (gconstpointer with_mount_monitor) /* Use bindfs, which does not need root privileges, to mount the contents of one dir * into another dir (and do the mount as readonly as per passed '-o ro' option) */ - run (&error, bindfs, "-n", "-o", "ro", dir_to_mount, dir_mountpoint, NULL); - g_assert_no_error (error); + if (!run (&error, bindfs, "-n", "-o", "ro", dir_to_mount, dir_mountpoint, NULL)) + { + gchar *skip_message = g_strdup_printf ("Failed to run bindfs to set up test: %s", error->message); + g_test_skip (skip_message); + g_free (skip_message); + g_clear_error (&error); + return; + } /* Let's check now, that the file is in indeed in a readonly filesystem */ file_in_mountpoint = g_strdup_printf ("%s/example.txt", dir_mountpoint); From 6d0344fd9511a7700bfcfe422a41ad6a0dca7136 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Thu, 20 Feb 2020 17:43:49 +0200 Subject: [PATCH 17/17] GThread: Don't g_error() if setting the thread scheduler settings fails Instead only do a g_critical(). This is something that has to be fixed one way or another, but a g_critical() is less disruptive and makes sure that code that worked in previous GLib versions still works as bad as before. Fixes https://gitlab.gnome.org/GNOME/glib/issues/2039 --- glib/gthread-posix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/gthread-posix.c b/glib/gthread-posix.c index 271bb5dbc..29bc81a0f 100644 --- a/glib/gthread-posix.c +++ b/glib/gthread-posix.c @@ -1248,7 +1248,7 @@ linux_pthread_proxy (void *data) res = syscall (SYS_sched_setattr, tid, thread->scheduler_settings->attr, flags); errsv = errno; if (res == -1) - g_error ("Failed to set scheduler settings: %s", g_strerror (errsv)); + g_critical ("Failed to set scheduler settings: %s", g_strerror (errsv)); } return thread->proxy (data);