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