From d148fffb58935d69a20dcc42b3ce3d998742f0e7 Mon Sep 17 00:00:00 2001

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

Date: Fri, 13 Jul 2018 14:47:45 +0000

Subject: [PATCH] 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);

-- 

2.20.1