Merge lp:~stolowski/unity-scope-home/cancellables into lp:unity-scope-home

[Paweł Stołowski]

Cancel previous search on new one in same channel.

Cancel previous search on new one in same channel.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:86
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/39/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/39

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/39/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

34 + GLib.Timeout.add (int.max (20, 100/(scope_search.search_string.length + 1)), ()=> { return false; });

You need to yield for this to actually do something :)

review: Needs Fixing

[lp:~stolowski/unity-scope-home/cancellables]

87. By Paweł Stołowski on 2013-03-26

Merged more-async.

88. By Paweł Stołowski on 2013-03-26

Merged more-async.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:88
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/40/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/40

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/40/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

49 + }, cancellable, //TODO cancellable

Looks like that TODO is done :)

[lp:~stolowski/unity-scope-home/cancellables]

89. By Paweł Stołowski on 2013-03-27

Merged trunk.

90. By Paweł Stołowski on 2013-03-27

Yield after creating a Timeout source. Remove idle source on cancellation.

91. By Paweł Stołowski on 2013-03-27

Fix - one more removal of idle before cancellation.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:90
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/43/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/43

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/43/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:91
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/44/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/44

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/44/rebuild

review: Approve (continuous-integration)

[lp:~stolowski/unity-scope-home/cancellables]

92. By Paweł Stołowski on 2013-03-27

Use cancellable.set_error_if_cancelled to simplify idle source removal.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:92
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/45/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/45

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/45/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

57 + }, cancellable, sss_cb);

I'm a bit worried that there are no extra cancellable checks inside the SSSClient implementation, isn't it possible that it's cancelled after the query finishes but before the json parsing is done? In that case the callbacks will still be called and could cause a crash.

108 + debug ("The search for channel %s was cancelled", scope_search.channel_id);

Will be more useful if you also log the current search_query.

[lp:~stolowski/unity-scope-home/cancellables]

93. By Paweł Stołowski on 2013-03-27

Check cancellable before finalizing parsing in smart scopes search method.

[Paweł Stołowski (stolowski)]

> 57 + }, cancellable, sss_cb);
>
> I'm a bit worried that there are no extra cancellable checks inside the
> SSSClient implementation, isn't it possible that it's cancelled after the
> query finishes but before the json parsing is done? In that case the callbacks
> will still be called and could cause a crash.

Added an extra check before calling finalize_parsing (after soup request finished) - I think that's the only case that needs that.

>
> 108 + debug ("The search for channel %s was cancelled",
> scope_search.channel_id);
>
> Will be more useful if you also log the current search_query.

Done.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:93
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-ci/46/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-home-trunk-raring-amd64-ci/46

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-team-unity-scope-home-trunk-ci/46/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

Let's get this in.

review: Approve

=== modified file 'src/scope.vala'
--- src/scope.vala	2013-03-26 19:29:47 +0000
+++ src/scope.vala	2013-03-27 14:39:25 +0000
@@ -39,6 +39,7 @@
   private SmartScopes.ChannelIdMap channel_id_map = new SmartScopes.ChannelIdMap ();
   private uint metrics_timer;
   private int remote_scopes_retry_count = 0;
+  private HashTable<string, GLib.Cancellable?> search_cancel = new HashTable<string, GLib.Cancellable?> (str_hash, str_equal);
   private RemoteScopeRegistry remote_scope_registry;
 
   public HomeScope ()
@@ -412,6 +413,16 @@
 
     CategoryManager.instance ().observe (scope_search.channel_id, scope_search.results_model);
 
+    GLib.Cancellable? prev_search = search_cancel.lookup (scope_search.channel_id);
+    if (prev_search != null)
+    {
+      debug ("Cancelling previous search query for channel: %s", scope_search.channel_id);
+      prev_search.cancel ();
+    }
+
+    GLib.Cancellable cancellable = new GLib.Cancellable ();
+    search_cancel.insert (scope_search.channel_id, cancellable);
+
     bool wait_for_sss_query = false;
     bool wait_for_push = false;
     bool wait_for_search = false;
@@ -575,6 +586,21 @@
             remote_scopes_to_query += scope_id;
         }
 
+        // timeout to give cancellable a chance for update; it depends on query length (the longer the string, the shorter timeout)
+        GLib.Timeout.add (int.max (20, 100/(scope_search.search_string.length + 1)), ()=>
+        {
+          if (wait_for_search == false && wait_for_push == false && wait_for_sss_query == false)
+            search.callback ();
+          return false;
+        });
+        yield;
+
+        if (cancellable.is_cancelled ())
+        {
+          debug ("The search for '%s' on channel %s was cancelled", scope_search.search_string, scope_search.channel_id);
+          return;
+        }
+
         sss_client.search.begin (scope_search.search_string, session_id, remote_scopes_to_query, scope_mgr.disabled_scopes,
             (scope_id, row) =>
             {
@@ -607,8 +633,7 @@
               {
                 warning ("server_sid is null");
               }
-            }, null, //TODO cancellable
-            sss_cb);
+            }, cancellable, sss_cb);
       }
     }
     else
@@ -664,20 +689,30 @@
         yield;
       }
 
-      signal_categories_order (scope_search, recommended_search_scopes);
-
-      if (push_data_idle_src > 0)
+      try
       {
-        debug ("Flushing push results");
-        push_data_idle_cb (); //make sure we flush all push_results
-        if (pending_push_count > 0)
+        cancellable.set_error_if_cancelled ();
+
+        signal_categories_order (scope_search, recommended_search_scopes);
+
+        if (push_data_idle_src > 0)
         {
-          debug ("Waiting for results pushing to finish");
-          wait_for_push = true;
-          yield;
+          debug ("Flushing push results");
+          push_data_idle_cb (); //make sure we flush all push_results
+          if (pending_push_count > 0)
+          {
+            debug ("Waiting for results pushing to finish");
+            wait_for_push = true;
+            yield;
+          }
         }
-        Source.remove (push_data_idle_src);
+      }
+      finally
+      {
+        if (push_data_idle_src > 0)
+          Source.remove (push_data_idle_src);
         push_data_idle_src = 0;
+        debug ("The search for '%s' on channel %s was cancelled", scope_search.search_string, scope_search.channel_id);
       }
 
       debug ("Got %u recommended scopes from Smart Scope Service", recommended_search_scopes.length ());
@@ -745,6 +780,12 @@
       yield;
     }
     
+    if (cancellable.is_cancelled ())
+    {
+      debug ("The search for '%s' on channel %s was cancelled", scope_search.search_string, scope_search.channel_id);
+      return;
+    }
+
     debug ("search finished");
 
     signal_categories_order (scope_search, recommended_search_scopes);

=== modified file 'src/smart-scopes-search.vala'
--- src/smart-scopes-search.vala	2013-03-22 19:40:37 +0000
+++ src/smart-scopes-search.vala	2013-03-27 14:39:25 +0000
@@ -114,6 +114,7 @@
           try
           {
             msg = yield queue_soup_message (session, msg, cancellable);
+            cancellable.set_error_if_cancelled ();
             if (msg.status_code == 200)
               finalize_parsing ();
           }
@@ -146,7 +147,6 @@
 
           b.set_member_name ("metadata");
 
-
           var content = result.metadata.lookup ("content"); //master scopes put 'metadata' in 'content'
           var metadata_var = content;