dee

Merge lp:~mhr3/dee/ignore-early-commits into lp:dee

Proposed by Michal Hruby on 2012-04-05
Status: Merged
Approved by: Michal Hruby on 2012-04-05
Approved revision: 366
Merged at revision: 363
Proposed branch: lp:~mhr3/dee/ignore-early-commits
Merge into: lp:dee
Diff against target: 216 lines (+104/-2)
4 files modified
doc/reference/dee-1.0/dee-1.0.types (+2/-0)
src/dee-shared-model.c (+15/-1)
tests/model-helper-clone3rows.c (+13/-1)
tests/test-model-interactions.c (+74/-0)
To merge this branch: bzr merge lp:~mhr3/dee/ignore-early-commits
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve on 2012-04-05
Gord Allott (community) 2012-04-05 Approve on 2012-04-05
Review via email: mp+100945@code.launchpad.net

Commit message

Ignore commits while we're waiting for Clone() reply

Description of the change

Turned out that all the issues we had with possible lenses crashes were caused by commit signals coming from the leader while a client was waiting for Clone to finish.

The issue had two parts - we were missing remote signal suppression after receiving a Clone reply - so when client's shared model got reset (right before applying the transaction from Clone reply), the clear got enqueued as new revisions which removed everything from the leader's model.

The second part is that we're blindly accepting any Commits while waiting for Clone to finish, which causes unnecessary invalidation of the model when Clone finishes (this is for example problem in HomeLens which doesn't really support removing of rows from some models - like category model, and can get into an odd state).

Added a synthetic test for the issue.

To post a comment you must log in.
Michal Hruby (mhr3) wrote :

As a note - without R366 (Ignore commits if we're waiting for clone to finish), the line:

117 + g_assert_cmpint (num_added, ==, 3);

will fail, although everything will be working.

Gord Allott (gordallott) wrote :

seems to make sense, but probably needs a dee person to take a real look

review: Approve

Looks correct. Deep stuff, and a good catch!

Also ran the extended test suite and can confirm the new test is passing.

review: Approve
Unity Merger (unity-merger) wrote :

No commit message specified.

Michal Hruby (mhr3) wrote :

@Mikkel, I can imagine this breaking your pipe hacks though. :(

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'doc/reference/dee-1.0/dee-1.0.types'
2--- doc/reference/dee-1.0/dee-1.0.types 2011-12-15 14:14:24 +0000
3+++ doc/reference/dee-1.0/dee-1.0.types 2012-04-05 12:01:26 +0000
4@@ -6,6 +6,7 @@
5 dee_hash_index_get_type
6 dee_index_get_type
7 dee_model_get_type
8+dee_model_iter_get_type
9 dee_peer_get_type
10 dee_proxy_model_get_type
11 dee_resource_manager_get_type
12@@ -14,6 +15,7 @@
13 dee_serializable_get_type
14 dee_serializable_model_get_type
15 dee_server_get_type
16+dee_shared_model_access_mode_get_type
17 dee_shared_model_get_type
18 dee_term_list_get_type
19 dee_text_analyzer_get_type
20
21=== modified file 'src/dee-shared-model.c'
22--- src/dee-shared-model.c 2012-03-26 08:43:27 +0000
23+++ src/dee-shared-model.c 2012-04-05 12:01:26 +0000
24@@ -100,6 +100,7 @@
25 gboolean synchronized;
26 gboolean found_first_peer;
27 gboolean suppress_remote_signals;
28+ gboolean clone_in_progress;
29
30 DeeSharedModelAccessMode access_mode;
31 };
32@@ -1018,7 +1019,11 @@
33 /* Guard against a race where we might inadvertedly have accepted a Commit
34 * before receiving the initial Clone */
35 if (dee_model_get_n_columns (DEE_MODEL (self)) > 0)
36+ {
37+ priv->suppress_remote_signals = TRUE;
38 reset_model (DEE_MODEL (self));
39+ priv->suppress_remote_signals = FALSE;
40+ }
41
42 /* We use the swarm name as sender_name here, because DBus passes us the
43 * unique name of the swarm leader here and we want to indicate in the debug
44@@ -1035,7 +1040,9 @@
45 priv->synchronized = TRUE;
46 g_object_notify (G_OBJECT (self), "synchronized");
47 }
48-
49+
50+ priv->clone_in_progress = FALSE;
51+
52 g_object_unref (self); // we grabbed a self ref during async call
53 }
54
55@@ -1072,6 +1079,8 @@
56 NULL, // cancel
57 on_clone_received, // cb
58 g_object_ref (self)); // userdata
59+
60+ priv->clone_in_progress = TRUE;
61 }
62 }
63
64@@ -1105,6 +1114,11 @@
65 if (g_strcmp0 (signal_name, "Commit") == 0)
66 {
67 model = DEE_SHARED_MODEL (user_data);
68+
69+ /* If we're waiting for Clone(), we can just ignore Commits coming
70+ * meanwhile, this way we'll prevent unnecessary invalidation */
71+ if (model->priv->clone_in_progress) return;
72+
73 disable_write = model->priv->access_mode ==
74 DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE;
75 forced_ignore = dee_peer_is_swarm_leader (model->priv->swarm) &&
76
77=== modified file 'tests/model-helper-clone3rows.c'
78--- tests/model-helper-clone3rows.c 2011-12-14 14:09:58 +0000
79+++ tests/model-helper-clone3rows.c 2012-04-05 12:01:26 +0000
80@@ -25,12 +25,20 @@
81 #include <gtx.h>
82 #include <dee.h>
83
84+static void
85+row_added (DeeModel *model, DeeModelIter *iter, gpointer data)
86+{
87+ gint *num_added = (gint*) data;
88+ (*num_added)++;
89+}
90+
91 /* Expects a clone with 3 rows in it */
92 gint
93 main (gint argc, gchar *argv[])
94 {
95 DeeModel *model;
96 DeeModelIter *iter;
97+ gint num_added;
98
99 g_type_init ();
100 g_thread_init (NULL);
101@@ -41,7 +49,10 @@
102 model = dee_shared_model_new (argv[1]);
103 else
104 model = dee_shared_model_new_for_peer ((DeePeer*) dee_client_new (argv[1]));
105-
106+
107+ num_added = 0;
108+ g_signal_connect (model, "row-added", G_CALLBACK (row_added), &num_added);
109+
110 if (gtx_wait_for_signal (G_OBJECT (model), 100000, "notify::synchronized", NULL))
111 g_error ("Helper model timed out waiting for 'ready' signal");
112
113@@ -60,6 +71,7 @@
114 g_assert_cmpstr (dee_model_get_string (model, iter, 1), == , "two");
115
116 gtx_assert_last_unref (model);
117+ g_assert_cmpint (num_added, ==, 3);
118
119 return 0;
120 }
121
122=== modified file 'tests/test-model-interactions.c'
123--- tests/test-model-interactions.c 2012-02-28 10:56:27 +0000
124+++ tests/test-model-interactions.c 2012-04-05 12:01:26 +0000
125@@ -58,6 +58,7 @@
126 static void test_ownership_stealing (Fixture *fix, gconstpointer data);
127 static void test_remote_append (Fixture *fix, gconstpointer data);
128 static void test_disabled_writes (Fixture *fix, gconstpointer data);
129+static void test_commit_before_clone (Fixture *fix, gconstpointer data);
130
131 void
132 test_model_interactions_create_suite (void)
133@@ -96,6 +97,8 @@
134 model_setup, test_remote_append, model_teardown);
135 g_test_add (DOMAIN"/DisabledWrites", Fixture, 0,
136 model_setup_null, test_disabled_writes, model_teardown_null);
137+ g_test_add (DOMAIN"/CommitBeforeClone", Fixture, 0,
138+ model_setup, test_commit_before_clone, model_teardown);
139 }
140
141 static void
142@@ -698,3 +701,74 @@
143 fix->model = NULL;
144 }
145
146+static gboolean
147+quit_loop (GMainLoop *ml)
148+{
149+ g_main_loop_quit (ml);
150+ return FALSE;
151+}
152+
153+static void
154+child_quit (GPid pid, gint status, gpointer user_data)
155+{
156+ GMainLoop *ml;
157+ gpointer *data;
158+
159+ data = user_data;
160+ ml = (GMainLoop*) data[0];
161+ data[1] = GINT_TO_POINTER (status);
162+
163+ g_main_loop_quit (ml);
164+}
165+
166+static void
167+test_commit_before_clone (Fixture *fix, gconstpointer data)
168+{
169+ GPid pid;
170+ GMainLoop *ml;
171+ gpointer *wait_data;
172+
173+ if (gtx_wait_for_signal (G_OBJECT (fix->model), TIMEOUT, "notify::synchronized", NULL))
174+ g_critical ("Model never emitted 'ready' signal");
175+
176+ _add3rows (fix->model);
177+
178+ /* need special handling of synchronization - commit must be emmitted after
179+ * the client is up (and possibly waiting for clone) */
180+
181+ g_spawn_async (TESTDIR,
182+ MODEL_HELPER (clone3rows, MODEL_NAME),
183+ NULL,
184+ G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH,
185+ NULL,
186+ NULL,
187+ &pid,
188+ NULL);
189+
190+ ml = g_main_loop_new (NULL, FALSE);
191+ wait_data = g_new0 (gpointer, 2);
192+ wait_data[0] = ml;
193+ wait_data[1] = GINT_TO_POINTER (837);
194+
195+ g_child_watch_add (pid, child_quit, wait_data);
196+
197+ /* wait for the helper process to start up, then flush the commits */
198+ g_usleep (500000);
199+
200+ /* don't wait indefinitely for the child */
201+ g_timeout_add (5000, (GSourceFunc) quit_loop, ml);
202+
203+ g_main_loop_run (ml);
204+
205+ if (GPOINTER_TO_INT (wait_data[1]) == 837)
206+ {
207+ g_critical ("Model helper timed out");
208+ }
209+ else if (GPOINTER_TO_INT (wait_data[1]) != 0)
210+ {
211+ g_critical ("Model helper returned error");
212+ }
213+
214+ g_assert_cmpint (dee_model_get_n_rows (fix->model), ==, 3);
215+}
216+

Subscribers

People subscribed via source and target branches

to all changes: