From 0986a258cc1df8c1e2aa17a0c2138d178405f902 Mon Sep 17 00:00:00 2001

From: Daniel Playfair Cal <daniel.playfair.cal@gmail.com>

Date: Wed, 25 Jul 2018 00:52:24 +1000

Subject: [PATCH 2/5] Engine: extend subscriptions state to account for

 multiple client subscriptions to the same path

Remove accidental whitespace change

Simplify branching in watch_fast and unwatch_fast

Indentation fixes

Store the subscription counts directly in the hash table pointer instead of mallocing ints

Add documentation comments for new utility functions

---

 engine/dconf-engine.c | 191 ++++++++++++++++++++++++++++--------------

 engine/dconf-engine.h |  11 ---

 tests/engine.c        |  54 +-----------

 3 files changed, 133 insertions(+), 123 deletions(-)

diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c

index 2a99eab..1963c34 100644

--- a/engine/dconf-engine.c

+++ b/engine/dconf-engine.c

@@ -170,8 +170,14 @@ struct _DConfEngine

   gchar              *last_handled;  /* reply tag from last item in in_flight */

-  GHashTable         *watched_paths; /* list of paths currently being watched for changes */

-  GHashTable         *pending_paths; /* list of paths waiting to enter watched state */

+  /**

+   * establishing and active, are hash tables storing the number

+   * of subscriptions to each path in the two possible states

+   */

+  /* active on the client side, but awaiting confirmation from the writer */

+  GHashTable         *establishing;

+  /* active on the client side, and with a D-Bus match rule established */

+  GHashTable         *active;

 };

 /* When taking the sources lock we check if any of the databases have

@@ -225,6 +231,78 @@ dconf_engine_unlock_queues (DConfEngine *engine)

   g_mutex_unlock (&engine->queue_lock);

 }

+/**

+ * Adds the count of subscriptions to @path in @from_table to the

+ * corresponding count in @to_table, creating it if it did not exist.

+ * Removes the count from @from_table.

+ */

+static void

+dconf_engine_move_subscriptions (GHashTable  *from_counts,

+                                 GHashTable  *to_counts,

+                                 const gchar *path)

+{

+  guint from_count = GPOINTER_TO_UINT (g_hash_table_lookup (from_counts, path));

+  guint old_to_count = GPOINTER_TO_UINT (g_hash_table_lookup (to_counts, path));

+  // Detect overflows

+  g_assert (old_to_count <= G_MAXUINT32 - from_count);

+  guint new_to_count = old_to_count + from_count;

+  if (from_count != 0)

+    {

+      g_hash_table_remove (from_counts, path);

+      g_hash_table_replace (to_counts,

+                            g_strdup (path),

+                            GUINT_TO_POINTER (new_to_count));

+    }

+}

+

+/**

+ * Increments the reference count for the subscription to @path, or sets

+ * it to 1 if it didn’t previously exist.

+ * Returns the new reference count.

+ */

+static guint

+dconf_engine_inc_subscriptions (GHashTable  *counts,

+                                const gchar *path)

+{

+  guint old_count = GPOINTER_TO_UINT (g_hash_table_lookup (counts, path));

+  // Detect overflows

+  g_assert (old_count < G_MAXUINT32);

+  guint new_count = old_count + 1;

+  g_hash_table_replace (counts, g_strdup (path), GUINT_TO_POINTER (new_count));

+  return new_count;

+}

+

+/**

+ * Decrements the reference count for the subscription to @path, or

+ * removes it if the new value is 0. The count must exist and be greater

+ * than 0.

+ * Returns the new reference count, or 0 if it does not exist.

+ */

+static guint

+dconf_engine_dec_subscriptions (GHashTable  *counts,

+                                const gchar *path)

+{

+  guint old_count = GPOINTER_TO_UINT (g_hash_table_lookup (counts, path));

+  g_assert (old_count > 0);

+  guint new_count = old_count - 1;

+  if (new_count == 0)

+    g_hash_table_remove (counts, path);

+  else

+    g_hash_table_replace (counts, g_strdup (path), GUINT_TO_POINTER (new_count));

+  return new_count;

+}

+

+/**

+ * Returns the reference count for the subscription to @path, or 0 if it

+ * does not exist.

+ */

+static guint

+dconf_engine_count_subscriptions (GHashTable  *counts,

+                                  const gchar *path)

+{

+  return GPOINTER_TO_UINT (g_hash_table_lookup (counts, path));

+}

+

 DConfEngine *

 dconf_engine_new (const gchar    *profile,

                   gpointer        user_data,

@@ -247,8 +325,14 @@ dconf_engine_new (const gchar    *profile,

   dconf_engine_global_list = g_slist_prepend (dconf_engine_global_list, engine);

   g_mutex_unlock (&dconf_engine_global_lock);

-  engine->watched_paths = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

-  engine->pending_paths = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);

+  engine->establishing = g_hash_table_new_full (g_str_hash,

+                                                g_str_equal,

+                                                g_free,

+                                                NULL);

+  engine->active = g_hash_table_new_full (g_str_hash,

+                                          g_str_equal,

+                                          g_free,

+                                          NULL);

   return engine;

 }

@@ -300,8 +384,8 @@ dconf_engine_unref (DConfEngine *engine)

       g_free (engine->sources);

-      g_hash_table_unref(engine->pending_paths);

-      g_hash_table_unref(engine->watched_paths);

+      g_hash_table_unref (engine->establishing);

+      g_hash_table_unref (engine->active);

       if (engine->free_func)

         engine->free_func (engine->user_data);

@@ -847,7 +931,14 @@ dconf_engine_watch_established (DConfEngine  *engine,

       dconf_engine_change_notify (engine, ow->path, changes, NULL, FALSE, NULL, engine->user_data);

     }

-  dconf_engine_set_watching (engine, ow->path, TRUE, TRUE);

+  guint num_establishing = dconf_engine_count_subscriptions (engine->establishing,

+                                                             ow->path);

+  if (num_establishing > 0)

+    // Subscription(s): establishing -> active

+    dconf_engine_move_subscriptions (engine->establishing,

+                                     engine->active,

+                                     ow->path);

+

   dconf_engine_call_handle_free (handle);

 }

@@ -855,14 +946,17 @@ void

 dconf_engine_watch_fast (DConfEngine *engine,

                          const gchar *path)

 {

-  if (dconf_engine_is_watching (engine, path, TRUE))

-    {

-      /**

-       * Either there is already a match rule in place for this exact path,

-       * or there is already a request in progress to add a match.

-       */

-      return;

-    }

+  guint num_establishing = dconf_engine_count_subscriptions (engine->establishing, path);

+  guint num_active = dconf_engine_count_subscriptions (engine->active, path);

+  if (num_active > 0)

+    // Subscription: inactive -> active

+    dconf_engine_inc_subscriptions (engine->active, path);

+  else

+    // Subscription: inactive -> establishing

+    num_establishing = dconf_engine_inc_subscriptions (engine->establishing,

+                                                       path);

+  if (num_establishing > 1 || num_active > 0)

+    return;

   OutstandingWatch *ow;

   gint i;

@@ -897,23 +991,33 @@ dconf_engine_watch_fast (DConfEngine *engine,

                                          "/org/freedesktop/DBus", "org.freedesktop.DBus", "AddMatch",

                                          dconf_engine_make_match_rule (engine->sources[i], path),

                                          &ow->handle, NULL);

-

-  dconf_engine_set_watching (engine, ow->path, TRUE, FALSE);

 }

 void

 dconf_engine_unwatch_fast (DConfEngine *engine,

                            const gchar *path)

 {

+  guint num_active = dconf_engine_count_subscriptions (engine->active, path);

+  guint num_establishing = dconf_engine_count_subscriptions (engine->establishing, path);

   gint i;

+  // Client code cannot unsubscribe if it is not subscribed

+  g_assert (num_active > 0 || num_establishing > 0);

+  if (num_active == 0)

+    // Subscription: establishing -> inactive

+    num_establishing = dconf_engine_dec_subscriptions (engine->establishing, path);

+  else

+    // Subscription: active -> inactive

+    num_active = dconf_engine_dec_subscriptions (engine->active, path);

+

+  if (num_active > 0 || num_establishing > 0)

+    return;

+

   for (i = 0; i < engine->n_sources; i++)

     if (engine->sources[i]->bus_type)

       dconf_engine_dbus_call_async_func (engine->sources[i]->bus_type, "org.freedesktop.DBus",

                                          "/org/freedesktop/DBus", "org.freedesktop.DBus", "RemoveMatch",

                                          dconf_engine_make_match_rule (engine->sources[i], path), NULL, NULL);

-

-  dconf_engine_set_watching (engine, path, FALSE, FALSE);

 }

 static void

@@ -951,16 +1055,18 @@ void

 dconf_engine_watch_sync (DConfEngine *engine,

                          const gchar *path)

 {

-  dconf_engine_handle_match_rule_sync (engine, "AddMatch", path);

-  dconf_engine_set_watching (engine, path, TRUE, TRUE);

+  guint num_active = dconf_engine_inc_subscriptions (engine->active, path);

+  if (num_active == 1)

+    dconf_engine_handle_match_rule_sync (engine, "AddMatch", path);

 }

 void

 dconf_engine_unwatch_sync (DConfEngine *engine,

                            const gchar *path)

 {

-  dconf_engine_handle_match_rule_sync (engine, "RemoveMatch", path);

-  dconf_engine_set_watching (engine, path, FALSE, FALSE);

+  guint num_active = dconf_engine_dec_subscriptions (engine->active, path);

+  if (num_active == 0)

+    dconf_engine_handle_match_rule_sync (engine, "RemoveMatch", path);

 }

 typedef struct

@@ -1418,42 +1524,3 @@ dconf_engine_sync (DConfEngine *engine)

     g_cond_wait (&engine->queue_cond, &engine->queue_lock);

   dconf_engine_unlock_queues (engine);

 }

-

-void

-dconf_engine_set_watching (DConfEngine    *engine,

-                           const gchar    *path,

-                           const gboolean  is_watching,

-                           const gboolean  is_established)

-{

-  if (is_watching)

-    {

-      if (is_established)

-        {

-          g_hash_table_add (engine->watched_paths, g_strdup (path));

-          g_hash_table_remove (engine->pending_paths, path);

-        }

-      else

-        {

-          g_hash_table_add (engine->pending_paths, g_strdup (path));

-          g_hash_table_remove (engine->watched_paths, path);

-        }

-    }

-  else

-    {

-      g_hash_table_remove (engine->watched_paths, path);

-      g_hash_table_remove (engine->pending_paths, path);

-    }

-}

-

-gboolean

-dconf_engine_is_watching (DConfEngine *engine, const gchar *path, const gboolean only_established)

-{

-  gconstpointer key = (gconstpointer) path;

-  if (g_hash_table_contains (engine->watched_paths, key))

-    return TRUE;

-

-  if (!only_established && g_hash_table_contains (engine->pending_paths, key))

-    return TRUE;

-

-  return FALSE;

-}

diff --git a/engine/dconf-engine.h b/engine/dconf-engine.h

index 06ed5a7..2485423 100644

--- a/engine/dconf-engine.h

+++ b/engine/dconf-engine.h

@@ -104,17 +104,6 @@ DConfEngine *           dconf_engine_new                                (const g

 G_GNUC_INTERNAL

 void                    dconf_engine_unref                              (DConfEngine             *engine);

-G_GNUC_INTERNAL

-void                    dconf_engine_set_watching                       (DConfEngine             *engine,

-                                                                         const gchar             *path,

-                                                                         const gboolean           is_watching,

-                                                                         const gboolean           is_established);

-

-G_GNUC_INTERNAL

-gboolean                dconf_engine_is_watching                        (DConfEngine             *engine,

-                                                                         const gchar             *path,

-                                                                         const gboolean           only_established);

-

 /* Read API: always handled immediately */

 G_GNUC_INTERNAL

 guint64                 dconf_engine_get_state                          (DConfEngine             *engine);

diff --git a/tests/engine.c b/tests/engine.c

index aa1db1c..038c04c 100644

--- a/tests/engine.c

+++ b/tests/engine.c

@@ -1210,13 +1210,16 @@ test_watch_fast (void)

   c = dconf_engine_get_state (engine);

   g_assert_cmpuint (b, ==, c);

   /* The watch result was not sent, because the path was already watched */

-  dconf_mock_dbus_assert_no_async();

+  dconf_mock_dbus_assert_no_async ();

   c = dconf_engine_get_state (engine);

   g_assert_cmpuint (b, ==, c);

   /* Since the path was already being watched,

    * do not expect a second false change notification */

   g_assert_cmpstr (change_log->str, ==, "/a/b/c:1::nil;");

   dconf_engine_unwatch_fast (engine, "/a/b/c");

+  /* nothing was done, because there is still a subscription left */

+  dconf_mock_dbus_assert_no_async ();

+  dconf_engine_unwatch_fast (engine, "/a/b/c");

   dconf_mock_dbus_async_reply (triv, NULL);

   dconf_mock_dbus_async_reply (triv, NULL);

   dconf_mock_dbus_assert_no_async ();

@@ -1286,54 +1289,6 @@ test_watch_sync (void)

   match_request_type = NULL;

 }

-static void

-test_watching (void)

-{

-  DConfEngine *engine;

-  const gchar *apple = "apple";

-  const gchar *orange = "orange";

-  const gchar *banana = "banana";

-

-  engine = dconf_engine_new (SRCDIR "/profile/dos", NULL, NULL);

-

-  g_assert (!dconf_engine_is_watching(engine, apple, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, apple, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, orange, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, orange, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, banana, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, banana, FALSE));

-

-  dconf_engine_set_watching (engine, apple, FALSE, FALSE);

-  dconf_engine_set_watching (engine, orange, TRUE, FALSE);

-  dconf_engine_set_watching (engine, banana, TRUE, TRUE);

-

-  g_assert (!dconf_engine_is_watching(engine, apple, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, apple, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, orange, TRUE));

-  g_assert (dconf_engine_is_watching(engine, orange, FALSE));

-  g_assert (dconf_engine_is_watching(engine, banana, TRUE));

-  g_assert (dconf_engine_is_watching(engine, banana, FALSE));

-

-  dconf_engine_set_watching (engine, orange, TRUE, TRUE);

-  dconf_engine_set_watching (engine, banana, FALSE, FALSE);

-

-  g_assert (!dconf_engine_is_watching(engine, apple, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, apple, FALSE));

-  g_assert (dconf_engine_is_watching(engine, orange, TRUE));

-  g_assert (dconf_engine_is_watching(engine, orange, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, banana, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, banana, FALSE));

-

-  dconf_engine_set_watching (engine, orange, FALSE, FALSE);

-

-  g_assert (!dconf_engine_is_watching(engine, apple, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, apple, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, orange, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, orange, FALSE));

-  g_assert (!dconf_engine_is_watching(engine, banana, TRUE));

-  g_assert (!dconf_engine_is_watching(engine, banana, FALSE));

-}

-

 static void

 test_change_fast (void)

 {

@@ -1819,7 +1774,6 @@ main (int argc, char **argv)

   g_test_add_func ("/engine/read", test_read);

   g_test_add_func ("/engine/watch/fast", test_watch_fast);

   g_test_add_func ("/engine/watch/sync", test_watch_sync);

-  g_test_add_func ("/engine/watch/watching", test_watching);

   g_test_add_func ("/engine/change/fast", test_change_fast);

   g_test_add_func ("/engine/change/sync", test_change_sync);

   g_test_add_func ("/engine/signals", test_signals);

-- 

2.20.1