[glib/th/gdbus-unsubscribe-right-away] gdbus: abort pending idle actions during g_dbus_connection_signal_unsubscribe()

[Thomas Haller <thaller src gnome org>]

commit 0d4f3345a7f2af44dceac06196a1420905cfa133
Author: Thomas Haller <thaller redhat com>
Date:   Wed Mar 31 21:09:47 2021 +0200

    gdbus: abort pending idle actions during g_dbus_connection_signal_unsubscribe()
    
    Since commit b285899a6fb ('gdbusconnection: Document main context iteration
    for unsubscriptions') we better document the behavior during signal
    unsubscribe.
    
    That is, after unsubscribe we are guaranteed that the callback will no
    longer be invoked (if you unsubscribe while the GMainContext is not
    iterated on another thread, otherwise there is obviously a race). But
    also, that unsubscribe does not immediately release all resources and
    that only after the GDestroyNotify is called (inside the GMainContext),
    that you can be sure that all resources are released. That is mostly
    important, because you must not stop iterating the GMainContext before,
    otherwise there will be leaks.
    
    Note that previously, you couldn't even be sure that if you scheduled an
    idle action after unsubscribe, that this idle action will be invoked
    before the GDestroyNotify. In particular this part I find the biggest
    problem. libnm's NMClient also needs to know when the signal subscription
    is completely wrapped up to not leak GMainContext. But it does not pass
    GDestroyNotify handlers to all the subscriptions that it makes. Instead,
    it schedules a last idle action with a lower priority. With this patch,
    it could also schedule the last cleanup action with G_PRIORITY_DEFAULT.
    
    That is undesirable behavior. It would be nice if we could release all
    resources right away. And we easily can.
    
    - if you call g_dbus_connection_signal_unsubscribe() form inside the
      GMainContext, then:
    
       - if you provide no GDestroyNotify, then that's it. It will all
         be cleaned up right way.
    
       - if you provide a GDestroyNotify, we will schedule it on an idle
         action right away (and that's the last thing that is to do).
         This means, if the user schedules another idle action after
         unsubscribe, they are guaranteed that the GDestroyNotify runs
         first.
    
    Note that for the most part this also performs better (not that
    performance was the goal of this patch).
    
    - we can abort unnecessary idle actions earlier before dispatching
      them.
    
    - we safe signal_subscriber_ref()/unref(), which was an atomic operation.
    
    - we still need a CONNECTION_LOCK() from   emit_signal_instance_in_idle_cb(),
      but we no longer need a hash lookup, merely some simple operations.
      Arguably, we trade a hash lookup with a g_source_unref(), which is
      an atomic operation and might be more expensive. Dunno.
    
    To be fair the biggest problem (see above) would also be solvable with a
    smaller patch. There was no need to have signal_subscriber_ref() and unref.
    During unsubscribe, we could always call call_destroy_notify() right
    away. Since idle actions preserve their order, there was no need to
    delay scheduling call_destroy_notify() after all other idle actions
    completed. But this solution is still better, because if the user
    doesn't provide a GDestroyNotify, then all resources will be released
    right away (when calling unsubscribe from inside the main context).

 gio/gdbusconnection.c | 88 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 36 deletions(-)
---
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c
index 740c65430..cbfb30bfe 100644
--- a/gio/gdbusconnection.c
+++ b/gio/gdbusconnection.c
@@ -95,6 +95,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "c-list.h"
 #include "gdbusauth.h"
 #include "gdbusutils.h"
 #include "gdbusaddress.h"
@@ -3267,13 +3268,15 @@ typedef struct
 
 typedef struct
 {
-  /* All fields are immutable after construction. */
-  gatomicrefcount ref_count;
+  /* All fields are immutable after construction,
+   *
+   * ... except sigsubscr_lst_head (which can only be accessed with a CONNECTION_LOCK). */
   GDBusSignalCallback callback;
   gpointer user_data;
   GDestroyNotify user_data_free_func;
   guint id;
   GMainContext *context;
+  CList sigsubscr_lst_head;
 } SignalSubscriber;
 
 typedef struct
@@ -3285,6 +3288,10 @@ typedef struct
   const gchar         *path;
   const gchar         *interface;
   const gchar         *member;
+
+  /* sigsubscr_lst and idle_source can only be accessed with a CONNECTION_LOCK. */
+  CList                sigsubscr_lst;
+  GSource             *idle_source;
 } SignalInstance;
 
 static void
@@ -3301,29 +3308,33 @@ signal_data_free (SignalData *signal_data)
   g_free (signal_data);
 }
 
-static SignalSubscriber *
-signal_subscriber_ref (SignalSubscriber *subscriber)
-{
-  g_atomic_ref_count_inc (&subscriber->ref_count);
-  return subscriber;
-}
-
 static void
-signal_subscriber_unref (SignalSubscriber *subscriber)
-{
-  if (g_atomic_ref_count_dec (&subscriber->ref_count))
-    {
-      /* Destroy the user data. It doesn’t matter which thread
-       * signal_subscriber_unref() is called in (or whether it’s called with a
-       * lock held), as call_destroy_notify() always defers to the next
-       * #GMainContext iteration. */
-      call_destroy_notify (subscriber->context,
-                           subscriber->user_data_free_func,
-                           subscriber->user_data);
-
-      g_main_context_unref (subscriber->context);
-      g_free (subscriber);
-    }
+signal_subscriber_destroy (SignalSubscriber *subscriber)
+{
+    SignalInstance *signal_instance;
+
+    while ((signal_instance = c_list_first_entry (&subscriber->sigsubscr_lst_head,
+                                                  SignalInstance,
+                                                  sigsubscr_lst)))
+      {
+        c_list_unlink (&signal_instance->sigsubscr_lst);
+        g_source_destroy (signal_instance->idle_source);
+        g_clear_pointer (&signal_instance->idle_source, g_source_unref);
+      }
+
+    /* Destroy the user data. It doesn’t matter which thread
+     * signal_subscriber_destroy() is called in (or whether it’s called with a
+     * lock held), as call_destroy_notify() always defers to the next
+     * #GMainContext iteration.
+     *
+     * This also ensures that SignalInstance->subscriber is alive while
+     * there are any idle actions pending. */
+    call_destroy_notify (subscriber->context,
+                         subscriber->user_data_free_func,
+                         subscriber->user_data);
+
+    g_main_context_unref (subscriber->context);
+    g_free (subscriber);
 }
 
 static gchar *
@@ -3576,12 +3587,12 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
     sender_unique_name = "";
 
   subscriber = g_new0 (SignalSubscriber, 1);
-  subscriber->ref_count = 1;
   subscriber->callback = callback;
   subscriber->user_data = user_data;
   subscriber->user_data_free_func = user_data_free_func;
   subscriber->id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */
   subscriber->context = g_main_context_ref_thread_default ();
+  c_list_init (&subscriber->sigsubscr_lst_head);
 
   /* see if we've already have this rule */
   signal_data = g_hash_table_lookup (connection->map_rule_to_signal_data, rule);
@@ -3601,7 +3612,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection     *connection,
   signal_data->object_path           = g_strdup (object_path);
   signal_data->arg0                  = g_strdup (arg0);
   signal_data->flags                 = flags;
-  signal_data->subscribers           = g_ptr_array_new_with_free_func ((GDestroyNotify) 
signal_subscriber_unref);
+  signal_data->subscribers           = g_ptr_array_new_with_free_func ((GDestroyNotify) 
signal_subscriber_destroy);
   g_ptr_array_add (signal_data->subscribers, subscriber);
 
   g_hash_table_insert (connection->map_rule_to_signal_data,
@@ -3765,7 +3776,7 @@ emit_signal_instance_in_idle_cb (gpointer data)
 {
   SignalInstance *signal_instance = data;
   GVariant *parameters;
-  gboolean has_subscription;
+  gboolean still_alive;
 
   parameters = g_dbus_message_get_body (signal_instance->message);
   if (parameters == NULL)
@@ -3788,15 +3799,19 @@ emit_signal_instance_in_idle_cb (gpointer data)
            g_variant_print (parameters, TRUE));
 #endif
 
-  /* Careful here, don't do the callback if we no longer has the subscription */
+  /* We need to check whether signal_instance was not unsubscribed
+   * in the meantime. In that case, we no longer invoke the callback. */
   CONNECTION_LOCK (signal_instance->connection);
-  has_subscription = FALSE;
-  if (g_hash_table_lookup (signal_instance->connection->map_id_to_signal_data,
-                           GUINT_TO_POINTER (signal_instance->subscriber->id)) != NULL)
-    has_subscription = TRUE;
+  still_alive = (!!signal_instance->idle_source);
+  if (still_alive)
+    c_list_unlink (&signal_instance->sigsubscr_lst);
   CONNECTION_UNLOCK (signal_instance->connection);
 
-  if (has_subscription)
+  /* let's unref idle_source outside the lock. No other thread will access
+   * @signal_instance after we unlinked sigsubscr_lst. So doing it here is fine. */
+  g_clear_pointer (&signal_instance->idle_source, g_source_unref);
+
+  if (still_alive)
     signal_instance->subscriber->callback (signal_instance->connection,
                                            signal_instance->sender,
                                            signal_instance->path,
@@ -3815,7 +3830,6 @@ signal_instance_free (SignalInstance *signal_instance)
 {
   g_object_unref (signal_instance->message);
   g_object_unref (signal_instance->connection);
-  signal_subscriber_unref (signal_instance->subscriber);
   g_free (signal_instance);
 }
 
@@ -3937,7 +3951,7 @@ schedule_callbacks (GDBusConnection *connection,
           SignalInstance *signal_instance;
 
           signal_instance = g_new0 (SignalInstance, 1);
-          signal_instance->subscriber = signal_subscriber_ref (subscriber);
+          signal_instance->subscriber = subscriber;
           signal_instance->message = g_object_ref (message);
           signal_instance->connection = g_object_ref (connection);
           signal_instance->sender = sender;
@@ -3953,7 +3967,9 @@ schedule_callbacks (GDBusConnection *connection,
                                  (GDestroyNotify) signal_instance_free);
           g_source_set_name (idle_source, "[gio] emit_signal_instance_in_idle_cb");
           g_source_attach (idle_source, subscriber->context);
-          g_source_unref (idle_source);
+          signal_instance->idle_source = idle_source;
+
+          c_list_link_tail (&subscriber->sigsubscr_lst_head, &signal_instance->sigsubscr_lst);
         }
     }
 }