From d148fffb58935d69a20dcc42b3ce3d998742f0e7 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal 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