Merge lp:~mhr3/libunity/remote-scope-fixes into lp:libunity

[Michal Hruby]

Restart crashed remote scopes and fix a couple of warnings when they do crash

There were a couple of issues with remote scopes:

1) We didn't restart them if they crashed
2) After the scope disappeared we tried to manipulate a filter model that was unreferenced
3) ScopeError was not registered as dbus error on first call to search/global_search

These issues caused warnings when a remote scope was killed and they are gone after applying these fixes.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Looks sensible. I am not sure if this is something we should test. Or I mean, we should, but if it's worth the (considerable) effort...

[Michal Hruby (mhr3)]

Yes, although we have tests where there's a lens and a remote scope talking to it, it's not registered as proper dbus service, so it can't be autostarted if it was killed. FWIW I tested all of this manually and the scopes restart now and warnings are gone, plus the changes don't touch any critical code paths for it to cause regressions.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Agreed. I took another detailed review of the code and I agree that it looks both correct and safe.

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-libunity/117/console reported an error when processing this lp:~mhr3/libunity/remote-scope-fixes branch.
Not merging it.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-libunity/118/console reported an error when processing this lp:~mhr3/libunity/remote-scope-fixes branch.
Not merging it.

=== modified file 'src/unity-lens-private.vala'
--- src/unity-lens-private.vala	2012-03-20 15:29:06 +0000
+++ src/unity-lens-private.vala	2012-04-18 10:48:17 +0000
@@ -265,8 +265,6 @@
   {
     ScopeProxy? scope = obj as ScopeProxy;
     
-    // FIXME: Remove existing receiver, if any
-    
     /* Because we need to manipulate the filters straight away,
      * we have to wait until it's synchronized
      */
@@ -279,6 +277,14 @@
     }
     else
       _filters_sync.add_receiver (scope.filters_model);
+
+    // make sure we unregister the filter_model if it dies
+    scope.filters_model.weak_ref ((WeakNotify) scope_filter_model_vanished);
+  }
+
+  private void scope_filter_model_vanished (Dee.Model filter_model)
+  {
+    _filters_sync.remove_receiver (filter_model);
   }
 
   private void on_scope_sources_updated (Object obj, ParamSpec pspec)

=== modified file 'src/unity-lens-tools.vala'
--- src/unity-lens-tools.vala	2012-03-16 16:33:15 +0000
+++ src/unity-lens-tools.vala	2012-04-18 10:48:17 +0000
@@ -63,14 +63,29 @@
 
   public void add_receiver (Dee.Model receiver)
   {
-    receiver.clear();
+    if (receiver in _receivers) return;
+    receiver.clear ();
     
     for (int i = 0; i < provider.get_n_rows(); ++i)
     {
       unowned Dee.ModelIter iter = provider.get_iter_at_row(i);
       add_row(iter, receiver);
     }
-     _receivers.add(receiver);
+
+    _receivers.add (receiver);
+  }
+
+  public void remove_receiver (Dee.Model receiver)
+  {
+    int index = _receivers.index_of (receiver);
+    if (index >= 0)
+    {
+      _receivers.remove_at (index);
+    }
+    else
+    {
+      warning ("Receiver [%p] was not registered", receiver);
+    }
   }
 
   private void on_row_added (Dee.Model model, Dee.ModelIter iter)

=== modified file 'src/unity-scope-interface.vala'
--- src/unity-scope-interface.vala	2012-01-06 13:04:27 +0000
+++ src/unity-scope-interface.vala	2012-04-18 10:48:17 +0000
@@ -68,14 +68,16 @@
 {
   public abstract async void info_request () throws IOError;
 
-  public abstract async ActivationReplyRaw activate (string uri,
-                                                     uint action_type) throws IOError;
+  public abstract async ActivationReplyRaw activate (
+      string uri, uint action_type) throws IOError;
 
   public abstract async HashTable<string, Variant> search (
-      string search_string, HashTable<string, Variant> hints) throws Error;
+      string search_string,
+      HashTable<string, Variant> hints) throws IOError, ScopeError;
 
   public abstract async HashTable<string, Variant> global_search (
-      string search_string, HashTable<string, Variant> hints) throws Error;
+      string search_string,
+      HashTable<string, Variant> hints) throws IOError, ScopeError;
   
   public abstract async PreviewReplyRaw preview (string uri) throws IOError;
     

=== modified file 'src/unity-scope-private.vala'
--- src/unity-scope-private.vala	2012-02-13 19:33:59 +0000
+++ src/unity-scope-private.vala	2012-04-18 10:48:17 +0000
@@ -367,7 +367,7 @@
 
   private async HashTable<string, Variant> search_internal (
       string search_string, HashTable<string, Variant> hints,
-      SearchType search_type) throws Error
+      SearchType search_type) throws ScopeError
   {
     HashTable<string, Variant> result =
         new HashTable<string, Variant> (str_hash, str_equal);
@@ -468,8 +468,8 @@
     return result;
   }
 
-  public async HashTable<string, Variant> search (
-      string search_string, HashTable<string, Variant> hints) throws Error
+  public async HashTable<string, Variant> search (string search_string,
+      HashTable<string, Variant> hints) throws IOError, ScopeError
   {
     HashTable<string, Variant> result;
 
@@ -478,8 +478,8 @@
     return result;
   }
 
-  public async HashTable<string, Variant> global_search (
-      string search_string, HashTable<string, Variant> hints) throws Error
+  public async HashTable<string, Variant> global_search (string search_string,
+      HashTable<string, Variant> hints) throws IOError, ScopeError
   {
     HashTable<string, Variant> result;
 

=== modified file 'src/unity-scope-proxy-remote.vala'
--- src/unity-scope-proxy-remote.vala	2012-03-16 16:48:13 +0000
+++ src/unity-scope-proxy-remote.vala	2012-04-18 10:48:17 +0000
@@ -69,6 +69,8 @@
   private Dee.SharedModel? _filters_model;
   private uint _reconnection_id = 0;
   private bool synchronized = false;
+  private int64 _last_scope_crash = 0;
+  private uint _scope_crashes = 0;
 
   public ScopeProxyRemote (string dbus_name_, string dbus_path_)
   {
@@ -118,7 +120,6 @@
   private void on_scope_vanished (DBusConnection conn,
                                   string         name)
   {
-    /* FIXME: Add some protection against Scopes that keep restarting */
     Trace.log_object (this, "Scope vanished: %s", dbus_name);
 
     synchronized = false;
@@ -130,10 +131,38 @@
     if (_global_results_model != null)
       _global_results_model.clear ();
 
-    if (_filters_model != null)
-      _filters_model.clear ();
-
-    start_reconnection_timeout ();
+    /* No need to clear the filters model, it's read-only for the scope and
+     * it would just cause warnings from filters synchronizer */
+    _filters_model = null;
+
+    if (_service != null)
+    {
+      /* Here comes the protected-restarting logic - the scope will be
+       * restarted unless it crashed more than 10 times during the past
+       * 15 minutes */
+      _scope_crashes++;
+      var cur_time = get_monotonic_time ();
+      var time_since_last_crash = cur_time - _last_scope_crash;
+
+      if (time_since_last_crash >= 15*60000000) // 15 minutes
+      {
+        _last_scope_crash = cur_time;
+        // reset crash counter, it's not that bad
+        _scope_crashes = 1;
+      }
+      else if (_scope_crashes >= 10)
+      {
+        // more than 10 crashes in the past 15 minutes
+        warning ("Scope %s is crashing too often, disabling it", dbus_name);
+        return;
+      }
+
+      start_reconnection_timeout ();
+    }
+    else
+    {
+      start_reconnection_timeout ();
+    }
   }
 
   private void start_reconnection_timeout ()
@@ -141,10 +170,12 @@
     if (_reconnection_id != 0)
       Source.remove (_reconnection_id);
 
-    _reconnection_id = Timeout.add_seconds (1, () =>
+    _reconnection_id = Timeout.add_seconds (2, () =>
     {
       if (_service == null)
         connect_to_scope ();
+      else if ((_service as DBusProxy).g_name_owner == null)
+        _service.info_request (); // ping the service to autostart it
 
       _reconnection_id = 0;
       return false;