From 21711aa40bed4e61bba7d5f9fee141825fe76823 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Thu, 26 Jul 2018 00:00:09 +1000 Subject: [PATCH 4/5] Engine: Add locks around access to subscription counts to ensure that each state transition is atomic Update comment about threading, documenting the new lock Add documentation comments for new utility functions --- engine/dconf-engine.c | 47 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 3 deletions(-) diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c index 2911724..ad891e6 100644 --- a/engine/dconf-engine.c +++ b/engine/dconf-engine.c @@ -128,7 +128,7 @@ * it is willing to deal with receiving the change notifies in those * threads. * - * Thread-safety is implemented using two locks. + * Thread-safety is implemented using three locks. * * The first lock (sources_lock) protects the sources. Although the * sources are only ever read from, it is necessary to lock them because @@ -143,8 +143,15 @@ * The second lock (queue_lock) protects the various queues that are * used to implement the "fast" writes described above. * - * If both locks are held at the same time then the sources lock must - * have been acquired first. + * The third lock (subscription_count_lock) protects the two hash tables + * that are used to keep track of the number of subscriptions held by + * the client library to each path. + * + * If sources_lock and queue_lock are held at the same time then then + * sources_lock must have been acquired first. + * + * subscription_count_lock is never held at the same time as + * sources_lock or queue_lock */ #define MAX_IN_FLIGHT 2 @@ -174,6 +181,8 @@ struct _DConfEngine * establishing and active, are hash tables storing the number * of subscriptions to each path in the two possible states */ + /* This lock ensures that transactions involving subscription counts are atomic */ + GMutex subscription_count_lock; /* 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 */ @@ -303,6 +312,25 @@ dconf_engine_count_subscriptions (GHashTable *counts, return GPOINTER_TO_UINT (g_hash_table_lookup (counts, path)); } +/** + * Acquires the subscription counts lock, which must be held when + * reading or writing to the subscription counts. + */ +static void +dconf_engine_lock_subscription_counts (DConfEngine *engine) +{ + g_mutex_lock (&engine->subscription_count_lock); +} + +/** + * Releases the subscription counts lock + */ +static void +dconf_engine_unlock_subscription_counts (DConfEngine *engine) +{ + g_mutex_unlock (&engine->subscription_count_lock); +} + DConfEngine * dconf_engine_new (const gchar *profile, gpointer user_data, @@ -325,6 +353,7 @@ 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); + g_mutex_init (&engine->subscription_count_lock); engine->establishing = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, @@ -387,6 +416,8 @@ dconf_engine_unref (DConfEngine *engine) g_hash_table_unref (engine->establishing); g_hash_table_unref (engine->active); + g_mutex_clear (&engine->subscription_count_lock); + if (engine->free_func) engine->free_func (engine->user_data); @@ -932,6 +963,7 @@ dconf_engine_watch_established (DConfEngine *engine, dconf_engine_change_notify (engine, ow->path, changes, NULL, FALSE, NULL, engine->user_data); } + dconf_engine_lock_subscription_counts (engine); guint num_establishing = dconf_engine_count_subscriptions (engine->establishing, ow->path); g_debug ("watch_established: \"%s\" (establishing: %d)", ow->path, num_establishing); @@ -941,6 +973,7 @@ dconf_engine_watch_established (DConfEngine *engine, engine->active, ow->path); + dconf_engine_unlock_subscription_counts (engine); dconf_engine_call_handle_free (handle); } @@ -948,6 +981,7 @@ void dconf_engine_watch_fast (DConfEngine *engine, const gchar *path) { + dconf_engine_lock_subscription_counts (engine); guint num_establishing = dconf_engine_count_subscriptions (engine->establishing, path); guint num_active = dconf_engine_count_subscriptions (engine->active, path); g_debug ("watch_fast: \"%s\" (establishing: %d, active: %d)", path, num_establishing, num_active); @@ -958,6 +992,7 @@ dconf_engine_watch_fast (DConfEngine *engine, // Subscription: inactive -> establishing num_establishing = dconf_engine_inc_subscriptions (engine->establishing, path); + dconf_engine_unlock_subscription_counts (engine); if (num_establishing > 1 || num_active > 0) return; @@ -1000,6 +1035,7 @@ void dconf_engine_unwatch_fast (DConfEngine *engine, const gchar *path) { + dconf_engine_lock_subscription_counts (engine); guint num_active = dconf_engine_count_subscriptions (engine->active, path); guint num_establishing = dconf_engine_count_subscriptions (engine->establishing, path); gint i; @@ -1014,6 +1050,7 @@ dconf_engine_unwatch_fast (DConfEngine *engine, // Subscription: active -> inactive num_active = dconf_engine_dec_subscriptions (engine->active, path); + dconf_engine_unlock_subscription_counts (engine); if (num_active > 0 || num_establishing > 0) return; @@ -1059,7 +1096,9 @@ void dconf_engine_watch_sync (DConfEngine *engine, const gchar *path) { + dconf_engine_lock_subscription_counts (engine); guint num_active = dconf_engine_inc_subscriptions (engine->active, path); + dconf_engine_unlock_subscription_counts (engine); g_debug ("watch_sync: \"%s\" (active: %d)", path, num_active - 1); if (num_active == 1) dconf_engine_handle_match_rule_sync (engine, "AddMatch", path); @@ -1069,7 +1108,9 @@ void dconf_engine_unwatch_sync (DConfEngine *engine, const gchar *path) { + dconf_engine_lock_subscription_counts (engine); guint num_active = dconf_engine_dec_subscriptions (engine->active, path); + dconf_engine_unlock_subscription_counts (engine); g_debug ("unwatch_sync: \"%s\" (active: %d)", path, num_active + 1); if (num_active == 0) dconf_engine_handle_match_rule_sync (engine, "RemoveMatch", path); -- 2.20.1