[dconf: 2/6] Engine: extend subscriptions state to account for multiple client subscriptions to the same path
- From: Daniel Playfair Cal <danielplayfaircal src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [dconf: 2/6] Engine: extend subscriptions state to account for multiple client subscriptions to the same path
- Date: Mon, 13 Aug 2018 23:49:44 +0000 (UTC)
commit 0986a258cc1df8c1e2aa17a0c2138d178405f902
Author: Daniel Playfair Cal <daniel playfair cal gmail com>
Date: Wed Jul 25 00:52:24 2018 +1000
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);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
