From 21711aa40bed4e61bba7d5f9fee141825fe76823 Mon Sep 17 00:00:00 2001
From: Daniel Playfair Cal <daniel.playfair.cal@gmail.com>
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