[glib/gdbus] Don't hold lock when doing callbacks for watching/owning names

[David Zeuthen <davidz src gnome org>]

commit e8074c592b05ddb6567527b32510ecdbde4b05f4
Author: David Zeuthen <davidz redhat com>
Date:   Sat May 2 14:16:00 2009 -0400

    Don't hold lock when doing callbacks for watching/owning names
---
 gdbus/gdbusnameowning.c   |  110 ++++++++++++++++++++++++++++++---------------
 gdbus/gdbusnamewatching.c |  107 +++++++++++++++++++++++++++++--------------
 2 files changed, 146 insertions(+), 71 deletions(-)

diff --git a/gdbus/gdbusnameowning.c b/gdbus/gdbusnameowning.c
index aef8f81..2ec71df 100644
--- a/gdbus/gdbusnameowning.c
+++ b/gdbus/gdbusnameowning.c
@@ -33,8 +33,6 @@
 
 #include "gdbusalias.h"
 
-/* TODO: don't hold lock when doing callbacks */
-
 /**
  * SECTION:gdbusnameowning
  * @title: Owning Bus Names
@@ -59,6 +57,7 @@ typedef enum
 
 typedef struct
 {
+  volatile gint             ref_count;
   guint                     id;
   GBusNameAcquiredCallback  name_acquired_handler;
   GBusNameLostCallback      name_lost_handler;
@@ -72,24 +71,37 @@ typedef struct
 
   gulong                    is_initialized_notify_id;
   guint                     idle_id;
+
+  gboolean                  cancelled;
 } Client;
 
 static guint next_global_id = 1;
 static GHashTable *map_id_to_client = NULL;
 
+
+static Client *
+client_ref (Client *client)
+{
+  g_atomic_int_inc (&client->ref_count);
+  return client;
+}
+
 static void
-client_free (Client *client)
+client_unref (Client *client)
 {
-  if (client->name_acquired_signal_handler_id > 0)
-    g_signal_handler_disconnect (client->owner, client->name_acquired_signal_handler_id);
-  if (client->name_lost_signal_handler_id > 0)
-    g_signal_handler_disconnect (client->owner, client->name_lost_signal_handler_id);
-  if (client->is_initialized_notify_id > 0)
-    g_signal_handler_disconnect (client->owner, client->is_initialized_notify_id);
-  if (client->idle_id > 0)
-    g_source_remove (client->idle_id);
-  g_object_unref (client->owner);
-  g_free (client);
+  if (g_atomic_int_dec_and_test (&client->ref_count))
+    {
+      if (client->name_acquired_signal_handler_id > 0)
+        g_signal_handler_disconnect (client->owner, client->name_acquired_signal_handler_id);
+      if (client->name_lost_signal_handler_id > 0)
+        g_signal_handler_disconnect (client->owner, client->name_lost_signal_handler_id);
+      if (client->is_initialized_notify_id > 0)
+        g_signal_handler_disconnect (client->owner, client->is_initialized_notify_id);
+      if (client->idle_id > 0)
+        g_source_remove (client->idle_id);
+      g_object_unref (client->owner);
+      g_free (client);
+    }
 }
 
 static void
@@ -124,11 +136,8 @@ on_name_lost (GBusNameOwner *owner,
 
 /* must only be called with lock held */
 static void
-client_is_determinate (Client *client)
+client_connect_signals (Client *client)
 {
-  gboolean owns_name;
-
-  /* connect signals */
   client->name_acquired_signal_handler_id = g_signal_connect (client->owner,
                                                               "name-acquired",
                                                               G_CALLBACK (on_name_acquired),
@@ -137,6 +146,12 @@ client_is_determinate (Client *client)
                                                               "name-lost",
                                                               G_CALLBACK (on_name_lost),
                                                               client);
+}
+/* must be called without lock held */
+static void
+client_do_initial_callbacks (Client *client)
+{
+  gboolean owns_name;
 
   /* and then report if the name is owned */
   owns_name = g_bus_name_owner_get_owns_name (client->owner);
@@ -166,15 +181,24 @@ client_is_determinate (Client *client)
 
 static void
 on_is_initialized_notify_cb (GBusNameOwner *owner,
-                             GParamSpec      *pspec,
-                             gpointer         user_data)
+                             GParamSpec    *pspec,
+                             gpointer       user_data)
 {
   Client *client = user_data;
   G_LOCK (lock);
   /* disconnect from signal */
+  if (client->cancelled)
+    goto out;
   g_signal_handler_disconnect (client->owner, client->is_initialized_notify_id);
   client->is_initialized_notify_id = 0;
-  client_is_determinate (client);
+  client_connect_signals (client);
+  G_UNLOCK (lock);
+  client_do_initial_callbacks (client);
+  client_unref (client);
+  return;
+
+ out:
+  client_unref (client);
   G_UNLOCK (lock);
 }
 
@@ -184,7 +208,16 @@ do_callback_in_idle (gpointer user_data)
   Client *client = user_data;
   G_LOCK (lock);
   client->idle_id = 0;
-  client_is_determinate (client);
+  if (client->cancelled)
+    goto out;
+  client_connect_signals (client);
+  G_UNLOCK (lock);
+  client_do_initial_callbacks (client);
+  client_unref (client);
+  return FALSE;
+
+ out:
+  client_unref (client);
   G_UNLOCK (lock);
   return FALSE;
 }
@@ -251,6 +284,7 @@ g_bus_own_name (GBusType                  bus_type,
   G_LOCK (lock);
 
   client = g_new0 (Client, 1);
+  client->ref_count = 1;
   client->id = next_global_id++; /* TODO: uh oh, handle overflow */
   client->name_acquired_handler = name_acquired_handler;
   client->name_lost_handler = name_lost_handler;
@@ -262,7 +296,7 @@ g_bus_own_name (GBusType                  bus_type,
       client->is_initialized_notify_id = g_signal_connect (client->owner,
                                                            "notify::is-initialized",
                                                            G_CALLBACK (on_is_initialized_notify_cb),
-                                                           client);
+                                                           client_ref (client));
     }
   else
     {
@@ -273,10 +307,7 @@ g_bus_own_name (GBusType                  bus_type,
 
   if (map_id_to_client == NULL)
     {
-      map_id_to_client = g_hash_table_new_full (g_direct_hash,
-                                                g_direct_equal,
-                                                NULL,
-                                                (GDestroyNotify) client_free);
+      map_id_to_client = g_hash_table_new (g_direct_hash, g_direct_equal);
     }
   g_hash_table_insert (map_id_to_client,
                        GUINT_TO_POINTER (client->id),
@@ -302,8 +333,9 @@ g_bus_unown_name (guint owner_id)
 {
   Client *client;
 
-  G_LOCK (lock);
+  client = NULL;
 
+  G_LOCK (lock);
   if (owner_id == 0 ||
       map_id_to_client == NULL ||
       (client = g_hash_table_lookup (map_id_to_client, GUINT_TO_POINTER (owner_id))) == NULL)
@@ -312,18 +344,24 @@ g_bus_unown_name (guint owner_id)
       goto out;
     }
 
-  if (client->previous_call == PREVIOUS_CALL_ACQUIRED)
-    {
-      if (client->name_lost_handler != NULL)
-        client->name_lost_handler (g_bus_name_owner_get_connection (client->owner),
-                                   g_bus_name_owner_get_name (client->owner),
-                                   client->user_data);
-    }
-
-  g_warn_if_fail (g_hash_table_remove (map_id_to_client, GUINT_TO_POINTER (owner_id)) == 1);
+  client->cancelled = TRUE;
+  g_warn_if_fail (g_hash_table_remove (map_id_to_client, GUINT_TO_POINTER (owner_id)));
 
  out:
   G_UNLOCK (lock);
+
+  /* do callback without holding lock */
+  if (client != NULL)
+    {
+      if (client->previous_call == PREVIOUS_CALL_ACQUIRED)
+        {
+          if (client->name_lost_handler != NULL)
+            client->name_lost_handler (g_bus_name_owner_get_connection (client->owner),
+                                       g_bus_name_owner_get_name (client->owner),
+                                       client->user_data);
+        }
+      client_unref (client);
+    }
 }
 
 #define __G_DBUS_NAME_OWNING_C__
diff --git a/gdbus/gdbusnamewatching.c b/gdbus/gdbusnamewatching.c
index a9c8733..08e5b0b 100644
--- a/gdbus/gdbusnamewatching.c
+++ b/gdbus/gdbusnamewatching.c
@@ -33,8 +33,6 @@
 
 #include "gdbusalias.h"
 
-/* TODO: don't hold lock when doing callbacks */
-
 /**
  * SECTION:gdbusnamewatching
  * @title: Watching Bus Names
@@ -59,6 +57,7 @@ typedef enum
 
 typedef struct
 {
+  volatile gint             ref_count;
   guint                     id;
   GBusNameAppearedCallback  name_appeared_handler;
   GBusNameVanishedCallback  name_vanished_handler;
@@ -72,24 +71,36 @@ typedef struct
 
   gulong                    is_initialized_notify_id;
   guint                     idle_id;
+
+  gboolean                  cancelled;
 } Client;
 
 static guint next_global_id = 1;
 static GHashTable *map_id_to_client = NULL;
 
+static Client *
+client_ref (Client *client)
+{
+  g_atomic_int_inc (&client->ref_count);
+  return client;
+}
+
 static void
-client_free (Client *client)
+client_unref (Client *client)
 {
-  if (client->name_appeared_signal_handler_id > 0)
-    g_signal_handler_disconnect (client->watcher, client->name_appeared_signal_handler_id);
-  if (client->name_vanished_signal_handler_id > 0)
-    g_signal_handler_disconnect (client->watcher, client->name_vanished_signal_handler_id);
-  if (client->is_initialized_notify_id > 0)
-    g_signal_handler_disconnect (client->watcher, client->is_initialized_notify_id);
-  if (client->idle_id > 0)
-    g_source_remove (client->idle_id);
-  g_object_unref (client->watcher);
-  g_free (client);
+  if (g_atomic_int_dec_and_test (&client->ref_count))
+    {
+      if (client->name_appeared_signal_handler_id > 0)
+        g_signal_handler_disconnect (client->watcher, client->name_appeared_signal_handler_id);
+      if (client->name_vanished_signal_handler_id > 0)
+        g_signal_handler_disconnect (client->watcher, client->name_vanished_signal_handler_id);
+      if (client->is_initialized_notify_id > 0)
+        g_signal_handler_disconnect (client->watcher, client->is_initialized_notify_id);
+      if (client->idle_id > 0)
+        g_source_remove (client->idle_id);
+      g_object_unref (client->watcher);
+      g_free (client);
+    }
 }
 
 static void
@@ -125,11 +136,8 @@ on_name_vanished (GBusNameWatcher *watcher,
 
 /* must only be called with lock held */
 static void
-client_is_determinate (Client *client)
+client_connect_signals (Client *client)
 {
-  const gchar *name_owner;
-
-  /* connect signals */
   client->name_appeared_signal_handler_id = g_signal_connect (client->watcher,
                                                               "name-appeared",
                                                               G_CALLBACK (on_name_appeared),
@@ -138,8 +146,14 @@ client_is_determinate (Client *client)
                                                               "name-vanished",
                                                               G_CALLBACK (on_name_vanished),
                                                               client);
+}
+
+/* must be called without lock held */
+static void
+client_do_initial_callbacks (Client *client)
+{
+  const gchar *name_owner;
 
-  /* and then report the name */
   name_owner = g_bus_name_watcher_get_name_owner (client->watcher);
   if (name_owner != NULL)
     {
@@ -174,9 +188,18 @@ on_is_initialized_notify_cb (GBusNameWatcher *watcher,
   Client *client = user_data;
   G_LOCK (lock);
   /* disconnect from signal */
+  if (client->cancelled)
+    goto out;
   g_signal_handler_disconnect (client->watcher, client->is_initialized_notify_id);
   client->is_initialized_notify_id = 0;
-  client_is_determinate (client);
+  client_connect_signals (client);
+  G_UNLOCK (lock);
+  client_do_initial_callbacks (client);
+  client_unref (client);
+  return;
+
+ out:
+  client_unref (client);
   G_UNLOCK (lock);
 }
 
@@ -186,7 +209,16 @@ do_callback_in_idle (gpointer user_data)
   Client *client = user_data;
   G_LOCK (lock);
   client->idle_id = 0;
-  client_is_determinate (client);
+  if (client->cancelled)
+    goto out;
+  client_connect_signals (client);
+  G_UNLOCK (lock);
+  client_do_initial_callbacks (client);
+  client_unref (client);
+  return FALSE;
+
+ out:
+  client_unref (client);
   G_UNLOCK (lock);
   return FALSE;
 }
@@ -251,6 +283,7 @@ g_bus_watch_name (GBusType                  bus_type,
 
   client = g_new0 (Client, 1);
   client->id = next_global_id++; /* TODO: uh oh, handle overflow */
+  client->ref_count = 1;
   client->name_appeared_handler = name_appeared_handler;
   client->name_vanished_handler = name_vanished_handler;
   client->user_data = user_data;
@@ -261,7 +294,7 @@ g_bus_watch_name (GBusType                  bus_type,
       client->is_initialized_notify_id = g_signal_connect (client->watcher,
                                                            "notify::is-initialized",
                                                            G_CALLBACK (on_is_initialized_notify_cb),
-                                                           client);
+                                                           client_ref (client));
     }
   else
     {
@@ -272,10 +305,7 @@ g_bus_watch_name (GBusType                  bus_type,
 
   if (map_id_to_client == NULL)
     {
-      map_id_to_client = g_hash_table_new_full (g_direct_hash,
-                                                g_direct_equal,
-                                                NULL,
-                                                (GDestroyNotify) client_free);
+      map_id_to_client = g_hash_table_new (g_direct_hash, g_direct_equal);
     }
   g_hash_table_insert (map_id_to_client,
                        GUINT_TO_POINTER (client->id),
@@ -301,8 +331,9 @@ g_bus_unwatch_name (guint watcher_id)
 {
   Client *client;
 
-  G_LOCK (lock);
+  client = NULL;
 
+  G_LOCK (lock);
   if (watcher_id == 0 ||
       map_id_to_client == NULL ||
       (client = g_hash_table_lookup (map_id_to_client, GUINT_TO_POINTER (watcher_id))) == NULL)
@@ -311,18 +342,24 @@ g_bus_unwatch_name (guint watcher_id)
       goto out;
     }
 
-  if (client->previous_call == PREVIOUS_CALL_APPEARED)
-    {
-      if (client->name_vanished_handler != NULL)
-        client->name_vanished_handler (g_bus_name_watcher_get_connection (client->watcher),
-                                       g_bus_name_watcher_get_name (client->watcher),
-                                       client->user_data);
-    }
-
-  g_warn_if_fail (g_hash_table_remove (map_id_to_client, GUINT_TO_POINTER (watcher_id)) == 1);
+  client->cancelled = TRUE;
+  g_warn_if_fail (g_hash_table_remove (map_id_to_client, GUINT_TO_POINTER (watcher_id)));
 
  out:
   G_UNLOCK (lock);
+
+  /* do callback without holding lock */
+  if (client != NULL)
+    {
+      if (client->previous_call == PREVIOUS_CALL_APPEARED)
+        {
+          if (client->name_vanished_handler != NULL)
+            client->name_vanished_handler (g_bus_name_watcher_get_connection (client->watcher),
+                                           g_bus_name_watcher_get_name (client->watcher),
+                                           client->user_data);
+        }
+      client_unref (client);
+    }
 }
 
 #define __G_DBUS_NAME_WATCHING_C__