Merge lp:~mhr3/libunity/shared-models-only-on-main-thread into lp:libunity

[Michal Hruby]

Fix a threading issue where shared model could have been serialized (in response to a standard Clone() request defined by the dee protocol) while a search was running in a separate thread and adding more rows to the model.

Fix a threading issue where shared model could have been serialized (in response to a standard Clone() request defined by the dee protocol) while a search was running in a separate thread add adding more rows to the model.

DeeSharedModels are no longer pushed to directly from the search thread, instead a IdleSource is used to add the rows from the main thread. This only fixes the non-deprecated scopes, but the deprecated ones don't perform searches in a separate thread in the first place.

[lp:~mhr3/libunity/shared-models-only-on-main-thread]

260. By Michal Hruby on 2013-07-17

Use more appropriate name in the tests

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:259
http://jenkins.qa.ubuntu.com/job/libunity-ci/67/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-amd64-ci/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-armhf-ci/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-i386-ci/52

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libunity-ci/67/rebuild

review: Approve (continuous-integration)

[lp:~mhr3/libunity/shared-models-only-on-main-thread]

261. By Michal Hruby on 2013-07-17

Bump valac requirement

[Marco Trevisan (Treviño) (3v1n0)]

Probably changing also build-dep for valac?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:261
http://jenkins.qa.ubuntu.com/job/libunity-ci/68/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-amd64-ci/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-armhf-ci/53
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-saucy-i386-ci/53

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libunity-ci/68/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

Looks good. I notice that it slightly changes the search time metric, but it probably won't make a substantial difference.

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2013-07-11 10:42:15 +0000
+++ configure.ac	2013-07-17 17:22:24 +0000
@@ -63,7 +63,7 @@
 AM_PROG_CC_STDC
 AC_HEADER_STDC
 AM_PROG_LIBTOOL
-AM_PROG_VALAC([0.16.0])
+AM_PROG_VALAC([0.18.0])
 AS_IF([test -z "$VALAC"], [AC_MSG_ERROR(["Can't find suitable vala compiler."])])
 
 ###################################################################
 
=== modified file 'src/unity-aggregator-scope-private.vala'
--- src/unity-aggregator-scope-private.vala	2013-07-16 17:23:39 +0000
+++ src/unity-aggregator-scope-private.vala	2013-07-17 17:22:24 +0000
@@ -1621,6 +1621,7 @@
 
     Unity.Trace.tracepoint ("%s run end: %s", Log.METHOD, owner.id);
 
+    result_set.flush ();
     response[SEARCH_SEQNUM_HINT] = new Variant.uint64 (channel.get_last_seqnum ());
     if (measure_requests)
     {
@@ -1629,8 +1630,6 @@
       response[SEARCH_TIME_HINT] = new Variant.double (delta);
     }
 
-    result_set.flush ();
-
     if (cancellable != null) cancellable.set_error_if_cancelled ();
     return response;
   }
 
=== modified file 'src/unity-deprecated-scope-impl.vala'
--- src/unity-deprecated-scope-impl.vala	2013-07-16 17:23:39 +0000
+++ src/unity-deprecated-scope-impl.vala	2013-07-17 17:22:24 +0000
@@ -503,7 +503,11 @@
       });
       yield;
 
-      result[SEARCH_SEQNUM_HINT] = new Variant.uint64 (channel.get_last_seqnum ());
+      if (!cancellable.is_cancelled ())
+      {
+        result_set.flush ();
+        result[SEARCH_SEQNUM_HINT] = new Variant.uint64 (channel.get_last_seqnum ());
+      }
 
       Unity.Trace.tracepoint ("%s run end: %s", Log.METHOD, owner.id);
       if (measure_requests)
@@ -513,11 +517,6 @@
         result[SEARCH_TIME_HINT] = new Variant.double (delta);
       }
 
-      if (!cancellable.is_cancelled ())
-      {
-        result_set.flush ();
-      }
-
       // handle hints
       var reply_hints = new_search.get_reply_hints ();
       if (reply_hints != null)
 
=== modified file 'src/unity-models.vala'
--- src/unity-models.vala	2013-07-15 09:56:14 +0000
+++ src/unity-models.vala	2013-07-17 17:22:24 +0000
@@ -35,6 +35,18 @@
     Object (results_model: model);
   }
 
+  private unowned Thread<void*> origin_thread;
+  private unowned MainContext origin_context;
+  private GenericArray<Variant> results;
+
+  construct
+  {
+    origin_thread = Thread.self<void*> ();
+    origin_context = MainContext.get_thread_default ();
+    if (origin_context == null) origin_context = MainContext.default ();
+    results = new GenericArray<Variant> ();
+  }
+
   public override void constructed ()
   {
     if (results_model == null)
@@ -59,33 +71,46 @@
     {
       metadata_v = new Variant.array (VariantType.VARDICT.element (), {});
     }
-    results_model.append (result.uri, result.icon_hint, result.category,
-                          result.result_type, result.mimetype, result.title,
-                          result.comment, result.dnd_uri, metadata_v);
+
+    Variant v = new Variant ("(ssuussss@a{sv})", result.uri, result.icon_hint,
+      result.category, result.result_type, result.mimetype, result.title,
+      result.comment, result.dnd_uri, metadata_v);
+    results.add ((owned) v);
   }
 
   public override void add_result_from_variant (Variant variant)
   {
-    if (variant.get_type_string () != "(ssuussssa{sv})")
+    const string EXPECTED_TYPE_STRING = "(ssuussssa{sv})";
+    if (variant.get_type_string () != EXPECTED_TYPE_STRING)
     {
-      warning ("Incorrect signature for %s", Log.METHOD);
+      warning ("Incorrect signature for %s, expected %s, but got %s",
+        Log.METHOD, EXPECTED_TYPE_STRING, variant.get_type_string ());
       return;
     }
 
-    Variant row_buf[9];
-    variant.get ("(@s@s@u@u@s@s@s@s@a{sv})",
-                 out row_buf[0], out row_buf[1],
-                 out row_buf[2], out row_buf[3],
-                 out row_buf[4], out row_buf[5],
-                 out row_buf[6], out row_buf[7],
-                 out row_buf[8]);
-    results_model.append_row (row_buf);
+    results.add (variant);
   }
 
   public Dee.SerializableModel flush_model { get; set; }
 
-  public override void flush ()
+  private void do_flush_locked ()
   {
+    uint results_len = (uint) results.length;
+    for (uint i = 0; i < results_len; i++)
+    {
+      Variant variant = (owned) results.data[i];
+      Variant row_buf[9];
+      variant.get ("(@s@s@u@u@s@s@s@s@a{sv})",
+                   out row_buf[0], out row_buf[1],
+                   out row_buf[2], out row_buf[3],
+                   out row_buf[4], out row_buf[5],
+                   out row_buf[6], out row_buf[7],
+                   out row_buf[8]);
+      results_model.append_row (row_buf);
+    }
+    // clear the results array
+    results.length = 0;
+
     var diff_model = flush_model as DiffModel;
     if (diff_model != null)
     {
@@ -97,6 +122,38 @@
       sm.flush_revision_queue ();
     }
   }
+
+  /* Note that this method can either be called from inside the search thread
+   * (while the search is in progress), or from the main thread BUT only after
+   * the search finished (if search is run in main thread it's safe always). */
+  public override void flush ()
+  {
+    // check which thread is this running in
+    unowned Thread<void*> current_thread = Thread.self<void*> ();
+    if (current_thread == origin_thread)
+    {
+      // no locking needed
+      do_flush_locked ();
+    }
+    else
+    {
+      // can't use Mutex and Cond directly, vala does scary copying of structs
+      var queue = new AsyncQueue<bool> ();
+      var idle = new IdleSource ();
+      idle.set_callback (() =>
+      {
+        do_flush_locked ();
+
+        queue.push (true);
+
+        return false;
+      });
+      idle.attach (origin_context);
+
+      // wait for the idle to get processed
+      queue.pop ();
+    }
+  }
 }
 
 internal class DiffModel: Dee.SharedModel
 
=== modified file 'src/unity-scope-dbus-impl.vala'
--- src/unity-scope-dbus-impl.vala	2013-07-16 17:23:39 +0000
+++ src/unity-scope-dbus-impl.vala	2013-07-17 17:22:24 +0000
@@ -549,18 +549,18 @@
 
       Unity.Trace.tracepoint ("%s run end: %s", Log.METHOD, _dbus_name);
 
-      result[SEARCH_SEQNUM_HINT] = new Variant.uint64 (channel.get_last_seqnum ());
+      // FIXME: handle no-reply-hint etc!
+      if (!cancellable.is_cancelled ())
+      {
+        result_set.flush ();
+        result[SEARCH_SEQNUM_HINT] = new Variant.uint64 (channel.get_last_seqnum ());
+      }
       if (measure_requests)
       {
         int64 delta_us = search_end_time - search_start_time;
         double delta = delta_us / 1000000.0;
         result[SEARCH_TIME_HINT] = new Variant.double (delta);
       }
-      // FIXME: handle no-reply-hint etc!
-      if (!cancellable.is_cancelled ())
-      {
-        result_set.flush ();
-      }
     }
     finally
     {
 
=== modified file 'test/vala/test-scope.vala'
--- test/vala/test-scope.vala	2013-07-16 17:24:10 +0000
+++ test/vala/test-scope.vala	2013-07-17 17:22:24 +0000
@@ -76,6 +76,8 @@
         Fixture.create<SimpleScopeTester> (SimpleScopeTester.test_simple_search));
     Test.add_data_func ("/Unit/SimpleScope/SearchAsync",
         Fixture.create<SimpleScopeTester> (SimpleScopeTester.test_simple_search_async));
+    Test.add_data_func ("/Unit/SimpleScope/SearchWithFlush",
+        Fixture.create<SimpleScopeTester> (SimpleScopeTester.test_simple_search_with_flush));
     Test.add_data_func ("/Unit/SimpleScope/DiffSearch",
         Fixture.create<SimpleScopeTester> (SimpleScopeTester.test_simple_diff_search));
     Test.add_data_func ("/Unit/SimpleScope/NoDiffSearch",
@@ -1015,7 +1017,31 @@
       assert (model.get_string (iter, 0) == "test:uri");
     }
 
-    public static void search_func (Unity.ScopeSearchBase search, int state = 0)
+    public void test_simple_search_with_flush ()
+    {
+      int search_invoked = 0;
+      // invoked in separate thread
+      simple.set_search_func ((search) =>
+      {
+        search_invoked++;
+
+        for (int i = 0; i <= 1; i++)
+        {
+          add_search_result (search, i);
+          search.search_context.result_set.flush ();
+        }
+      });
+
+      var reply_dict = perform_search ("");
+      assert (reply_dict != null);
+
+      assert (search_invoked == 1);
+      assert (model.get_n_rows () == 2);
+      var iter = model.get_first_iter ();
+      assert (model.get_string (iter, 0) == "test:uri");
+    }
+
+    public static void add_search_result (Unity.ScopeSearchBase search, int state = 0)
     {
       Unity.ScopeResult result = Unity.ScopeResult ();
       switch (state)
@@ -1033,17 +1059,6 @@
           search.search_context.result_set.add_result (result);
           break;
         case 1:
-          result.uri = "test:uri";
-          result.icon_hint = "";
-          result.category = 0;
-          result.result_type = Unity.ResultType.DEFAULT;
-          result.mimetype = "inode/folder";
-          result.title = "Title";
-          result.comment = "";
-          result.dnd_uri = "test::uri";
-          result.metadata = null;
-          search.search_context.result_set.add_result (result);
-
           result.uri = "test:uri2";
           result.icon_hint = "";
           result.category = 0;
@@ -1064,7 +1079,9 @@
       int search_state = 1;
       simple.set_search_func ((search) =>
       {
-        search_func (search, search_state);
+        add_search_result (search, 0);
+        if (search_state == 1)
+          add_search_result (search, 1);
       });
 
       var reply_dict = perform_search ("foo", ChannelFlags.DIFF_CHANGES);
@@ -1089,7 +1106,9 @@
       int search_state = 1;
       simple.set_search_func ((search) =>
       {
-        search_func (search, search_state);
+        add_search_result (search, 0);
+        if (search_state == 1)
+          add_search_result (search, 1);
       });
 
       var reply_dict = perform_search ("foo");