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

[Michal Hruby]

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

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.

[Michal Hruby (mhr3)]

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)]

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

review: Approve

[Mikkel Kamstrup Erlandsen (kamstrup)]

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)]

No commit message specified.

[Michal Hruby (mhr3)]

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

=== modified file 'doc/reference/dee-1.0/dee-1.0.types'
--- doc/reference/dee-1.0/dee-1.0.types	2011-12-15 14:14:24 +0000
+++ doc/reference/dee-1.0/dee-1.0.types	2012-04-05 12:01:26 +0000
@@ -6,6 +6,7 @@
 dee_hash_index_get_type
 dee_index_get_type
 dee_model_get_type
+dee_model_iter_get_type
 dee_peer_get_type
 dee_proxy_model_get_type
 dee_resource_manager_get_type
@@ -14,6 +15,7 @@
 dee_serializable_get_type
 dee_serializable_model_get_type
 dee_server_get_type
+dee_shared_model_access_mode_get_type
 dee_shared_model_get_type
 dee_term_list_get_type
 dee_text_analyzer_get_type

=== modified file 'src/dee-shared-model.c'
--- src/dee-shared-model.c	2012-03-26 08:43:27 +0000
+++ src/dee-shared-model.c	2012-04-05 12:01:26 +0000
@@ -100,6 +100,7 @@
   gboolean    synchronized;
   gboolean    found_first_peer;
   gboolean    suppress_remote_signals;
+  gboolean    clone_in_progress;
 
   DeeSharedModelAccessMode access_mode;
 };
@@ -1018,7 +1019,11 @@
       /* Guard against a race where we might inadvertedly have accepted a Commit
        * before receiving the initial Clone */
       if (dee_model_get_n_columns (DEE_MODEL (self)) > 0)
+      {
+        priv->suppress_remote_signals = TRUE;
         reset_model (DEE_MODEL (self));
+        priv->suppress_remote_signals = FALSE;
+      }
       
       /* We use the swarm name as sender_name here, because DBus passes us the
       * unique name of the swarm leader here and we want to indicate in the debug
@@ -1035,7 +1040,9 @@
       priv->synchronized = TRUE;
       g_object_notify (G_OBJECT (self), "synchronized");
     }
-  
+
+  priv->clone_in_progress = FALSE;
+
   g_object_unref (self); // we grabbed a self ref during async call
 }
 
@@ -1072,6 +1079,8 @@
                              NULL,                                   // cancel
                              on_clone_received,                      // cb
                              g_object_ref (self));                   // userdata
+
+      priv->clone_in_progress = TRUE;
     }
 }
 
@@ -1105,6 +1114,11 @@
   if (g_strcmp0 (signal_name, "Commit") == 0)
     {
       model = DEE_SHARED_MODEL (user_data);
+
+      /* If we're waiting for Clone(), we can just ignore Commits coming
+       * meanwhile, this way we'll prevent unnecessary invalidation */
+      if (model->priv->clone_in_progress) return;
+
       disable_write = model->priv->access_mode == 
         DEE_SHARED_MODEL_ACCESS_MODE_LEADER_WRITABLE;
       forced_ignore = dee_peer_is_swarm_leader (model->priv->swarm) &&

=== modified file 'tests/model-helper-clone3rows.c'
--- tests/model-helper-clone3rows.c	2011-12-14 14:09:58 +0000
+++ tests/model-helper-clone3rows.c	2012-04-05 12:01:26 +0000
@@ -25,12 +25,20 @@
 #include <gtx.h>
 #include <dee.h>
 
+static void
+row_added (DeeModel *model, DeeModelIter *iter, gpointer data)
+{
+  gint *num_added = (gint*) data;
+  (*num_added)++;
+}
+
 /* Expects a clone with 3 rows in it */
 gint
 main (gint argc, gchar *argv[])
 {
   DeeModel     *model;
   DeeModelIter *iter;
+  gint          num_added;
   
   g_type_init (); 
   g_thread_init (NULL);
@@ -41,7 +49,10 @@
     model = dee_shared_model_new (argv[1]);
   else
     model = dee_shared_model_new_for_peer ((DeePeer*) dee_client_new (argv[1]));
-  
+
+  num_added = 0;
+  g_signal_connect (model, "row-added", G_CALLBACK (row_added), &num_added);
+
   if (gtx_wait_for_signal (G_OBJECT (model), 100000, "notify::synchronized", NULL))
     g_error ("Helper model timed out waiting for 'ready' signal");
 
@@ -60,6 +71,7 @@
   g_assert_cmpstr (dee_model_get_string (model, iter, 1), == , "two");
   
   gtx_assert_last_unref (model);
+  g_assert_cmpint (num_added, ==, 3);
   
   return 0;
 }

=== modified file 'tests/test-model-interactions.c'
--- tests/test-model-interactions.c	2012-02-28 10:56:27 +0000
+++ tests/test-model-interactions.c	2012-04-05 12:01:26 +0000
@@ -58,6 +58,7 @@
 static void test_ownership_stealing (Fixture *fix, gconstpointer data);
 static void test_remote_append  (Fixture *fix, gconstpointer data);
 static void test_disabled_writes (Fixture *fix, gconstpointer data);
+static void test_commit_before_clone (Fixture *fix, gconstpointer data);
 
 void
 test_model_interactions_create_suite (void)
@@ -96,6 +97,8 @@
               model_setup, test_remote_append, model_teardown);
   g_test_add (DOMAIN"/DisabledWrites", Fixture, 0,
               model_setup_null, test_disabled_writes, model_teardown_null);
+  g_test_add (DOMAIN"/CommitBeforeClone", Fixture, 0,
+              model_setup, test_commit_before_clone, model_teardown);
 }
 
 static void
@@ -698,3 +701,74 @@
   fix->model = NULL;
 }
 
+static gboolean
+quit_loop (GMainLoop *ml)
+{
+  g_main_loop_quit (ml);
+  return FALSE;
+}
+
+static void
+child_quit (GPid pid, gint status, gpointer user_data)
+{
+  GMainLoop *ml;
+  gpointer *data;
+
+  data = user_data;
+  ml = (GMainLoop*) data[0];
+  data[1] = GINT_TO_POINTER (status);
+
+  g_main_loop_quit (ml);
+}
+
+static void
+test_commit_before_clone (Fixture *fix, gconstpointer data)
+{
+  GPid pid;
+  GMainLoop *ml;
+  gpointer *wait_data;
+
+  if (gtx_wait_for_signal (G_OBJECT (fix->model), TIMEOUT, "notify::synchronized", NULL))
+    g_critical ("Model never emitted 'ready' signal");
+
+  _add3rows (fix->model);
+
+  /* need special handling of synchronization - commit must be emmitted after
+   * the client is up (and possibly waiting for clone) */
+
+  g_spawn_async (TESTDIR,
+                 MODEL_HELPER (clone3rows, MODEL_NAME),
+                 NULL,
+                 G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH,
+                 NULL,
+                 NULL,
+                 &pid,
+                 NULL);
+
+  ml = g_main_loop_new (NULL, FALSE);
+  wait_data = g_new0 (gpointer, 2);
+  wait_data[0] = ml;
+  wait_data[1] = GINT_TO_POINTER (837);
+
+  g_child_watch_add (pid, child_quit, wait_data);
+
+  /* wait for the helper process to start up, then flush the commits */
+  g_usleep (500000);
+
+  /* don't wait indefinitely for the child */
+  g_timeout_add (5000, (GSourceFunc) quit_loop, ml);
+
+  g_main_loop_run (ml);
+
+  if (GPOINTER_TO_INT (wait_data[1]) == 837)
+  {
+    g_critical ("Model helper timed out");
+  }
+  else if (GPOINTER_TO_INT (wait_data[1]) != 0)
+  {
+    g_critical ("Model helper returned error");
+  }
+
+  g_assert_cmpint (dee_model_get_n_rows (fix->model), ==, 3);
+}
+