[dconf: 1/2] Engine: track in progress watch handles to avoid spurious changed signals for the root path

[Ray Strode <halfline src gnome org>]

commit d148fffb58935d69a20dcc42b3ce3d998742f0e7
Author: Daniel Playfair Cal <daniel playfair cal gmail com>
Date:   Fri Jul 13 14:47:45 2018 +0000

    Engine: track in progress watch handles to avoid spurious changed signals for the root path

 engine/dconf-engine.c | 88 ++++++++++++++++++++++++++++++++++++++++++++-------
 engine/dconf-engine.h | 11 +++++++
 tests/engine.c        | 66 ++++++++++++++++++++++++++++++++++++--
 3 files changed, 151 insertions(+), 14 deletions(-)
---
diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c
index bc36e52..c0ff12d 100644
--- a/engine/dconf-engine.c
+++ b/engine/dconf-engine.c
@@ -158,17 +158,20 @@ struct _DConfEngine
   GDestroyNotify      free_func;
   gint                ref_count;
 
-  GMutex              sources_lock; /* This lock is for the sources (ie: refreshing) and state. */
-  guint64             state;        /* Counter that changes every time a source is refreshed. */
-  DConfEngineSource **sources;      /* Array never changes, but each source changes internally. */
+  GMutex              sources_lock;  /* This lock is for the sources (ie: refreshing) and state. */
+  guint64             state;         /* Counter that changes every time a source is refreshed. */
+  DConfEngineSource **sources;       /* Array never changes, but each source changes internally. */
   gint                n_sources;
 
-  GMutex              queue_lock;   /* This lock is for pending, in_flight, queue_cond */
-  GCond               queue_cond;   /* Signalled when the queues empty */
-  GQueue              pending;      /* DConfChangeset */
-  GQueue              in_flight;    /* DConfChangeset */
+  GMutex              queue_lock;    /* This lock is for pending, in_flight, queue_cond */
+  GCond               queue_cond;    /* Signalled when the queues empty */
+  GQueue              pending;       /* DConfChangeset */
+  GQueue              in_flight;     /* DConfChangeset */
 
-  gchar              *last_handled; /* reply tag from last item in in_flight */
+  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 */
 };
 
 /* When taking the sources lock we check if any of the databases have
@@ -244,6 +247,9 @@ 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);
+
   return engine;
 }
 
@@ -799,8 +805,9 @@ typedef struct
 {
   DConfEngineCallHandle handle;
 
-  guint64 state;
-  gint    pending;
+  guint64  state;
+  gint     pending;
+  gchar   *path;
 } OutstandingWatch;
 
 static void
@@ -825,11 +832,13 @@ dconf_engine_watch_established (DConfEngine  *engine,
        * must have changed while our watch requests were on the wire.
        *
        * We don't know what changed, so we can just say that potentially
-       * everything changed.  This case is very rare, anyway...
+       * everything under the path being watched changed.  This case is
+       * very rare, anyway...
        */
-      dconf_engine_change_notify (engine, "/", changes, NULL, FALSE, NULL, engine->user_data);
+      dconf_engine_change_notify (engine, ow->path, changes, NULL, FALSE, NULL, engine->user_data);
     }
 
+  dconf_engine_set_watching (engine, ow->path, TRUE, TRUE);
   dconf_engine_call_handle_free (handle);
 }
 
@@ -837,6 +846,15 @@ 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;
+    }
+
   OutstandingWatch *ow;
   gint i;
 
@@ -855,6 +873,7 @@ dconf_engine_watch_fast (DConfEngine *engine,
   ow = dconf_engine_call_handle_new (engine, dconf_engine_watch_established,
                                      G_VARIANT_TYPE_UNIT, sizeof (OutstandingWatch));
   ow->state = dconf_engine_get_state (engine);
+  ow->path = g_strdup (path);
 
   /* We start getting async calls returned as soon as we start dispatching them,
    * so we must not touch the 'ow' struct after we send the first one.
@@ -869,6 +888,8 @@ 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
@@ -882,6 +903,8 @@ dconf_engine_unwatch_fast (DConfEngine *engine,
       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
@@ -920,6 +943,7 @@ 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);
 }
 
 void
@@ -927,6 +951,7 @@ 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);
 }
 
 typedef struct
@@ -1384,3 +1409,42 @@ 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 2485423..06ed5a7 100644
--- a/engine/dconf-engine.h
+++ b/engine/dconf-engine.h
@@ -104,6 +104,17 @@ 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 a804b9a..aa1db1c 100644
--- a/tests/engine.c
+++ b/tests/engine.c
@@ -1153,7 +1153,7 @@ test_watch_fast (void)
   DConfEngine *engine;
   GvdbTable *table;
   GVariant *triv;
-  guint64 a, b;
+  guint64 a, b, c;
 
   change_log = g_string_new (NULL);
 
@@ -1202,7 +1202,20 @@ test_watch_fast (void)
   dconf_mock_dbus_assert_no_async ();
   b = dconf_engine_get_state (engine);
   g_assert_cmpuint (a, !=, b);
-  g_assert_cmpstr (change_log->str, ==, "/:1::nil;");
+  g_assert_cmpstr (change_log->str, ==, "/a/b/c:1::nil;");
+  /* Try to establish a watch again for the same path */
+  dconf_engine_watch_fast (engine, "/a/b/c");
+  g_assert (!dconf_engine_has_outstanding (engine));
+  dconf_engine_sync (engine);
+  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();
+  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");
   dconf_mock_dbus_async_reply (triv, NULL);
   dconf_mock_dbus_async_reply (triv, NULL);
@@ -1273,6 +1286,54 @@ 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)
 {
@@ -1758,6 +1819,7 @@ 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);