Merge lp:~jamesh/libunity/export-multiple-scopes into lp:libunity

[James Henstridge]

Make ScopeDBusConnector.export() succeed if the process already owns the requested D-Bus name, making it possible to export multiple scopes from a process under the same bus name.

It is currently not possible to export two scopes from the same process using the new Unity.AbstractScope API: the export() call on the second ScopeDBusConnector will fail when it checks to see if the D-Bus name is already in use.

This branch changes the logic a bit, so rather than calling NameHasOwner(name) and erroring out if it returns true, I instead call GetNameOwner(name) and error out if the owner does not match our connection's unique name. If it does match our unique name, I export the scope. If the name has no owner, we go on to request the name as before.

The rationale for this behaviour is that if the process already owns the name, then it is likely to hold on to it for as long as the scope is exported.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:222
http://jenkins.qa.ubuntu.com/job/libunity-ci/12/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-raring-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-raring-armhf-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libunity-raring-i386-ci/12

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

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

As mentioned on IRC, this is a bit racy, if two scopes are defined to use the same name, in the time between the first acquiring the name and the second one checking whether the name is already owned, any method calls delivered for the second scope will fail.

We need something like a MultiScopeExporter, or otherwise change the export semantics for this to be safe enough.

review: Needs Fixing

[lp:~jamesh/libunity/export-multiple-scopes]

223. By James Henstridge on 2013-06-10

Merge from trunk

224. By James Henstridge on 2013-06-11

Defer acquisition of D-Bus names until Unity.ScopeDBusConnector.run() is
called, so we can make sure all scopes have been exported prior to
acquiring their associated names.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:224
http://jenkins.qa.ubuntu.com/job/libunity-ci/20/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-amd64-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-armhf-ci/5/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-i386-ci/5/console

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

review: Needs Fixing (continuous-integration)

[lp:~jamesh/libunity/export-multiple-scopes]

225. By James Henstridge on 2013-06-12

Move the private OwnedName struct to the internal namespace, and update
the symbols file.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:225
http://jenkins.qa.ubuntu.com/job/libunity-ci/21/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-amd64-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-armhf-ci/6/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libunity-saucy-i386-ci/6/console

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

review: Needs Fixing (continuous-integration)

[lp:~jamesh/libunity/export-multiple-scopes]

226. By James Henstridge on 2013-06-12

Update symbols.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

140 + public static bool acquire_names ()

Does this method really need to be public? I don't see why the loader should need to call it itself. Perhaps the run () method should be able to throw an error instead?

[Michal Hruby (mhr3)]

33 + public int ref_count;

Looks very low-level, could you turn the struct into a class with hold() and release() methods and unown the name on last release() call? Should make it more contained.

Also, this still looks like all names that need to be owned are known during the startup. That might not be the case, perhaps the loader starts with exporting a single scope and later another one with a different name is exported? What should the loader impl do in that case?

[James Henstridge (jamesh)]

I am using the acquire_names() call from the tests at present. It may also be useful from the loader for the load-scope-after-startup case, but I guess I can make it an internal symbol in the mean time to avoid extending the API.

The function is also designed so that it can be run multiple times too: if there is a handle for a particular name, then the name won't be requested again. So calling the function twice should be safe.

[lp:~jamesh/libunity/export-multiple-scopes]

227. By James Henstridge on 2013-06-17

Move the D-Bus name management into an internal helper class, and make
the acquire_names() call internal (can still be used by tests though).

228. By James Henstridge on 2013-06-17

Tidy up symbol exports.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

The branch has been updated to move the D-Bus name management into a separate ScopeDBusNameManager class, and the locking has been removed (based on discussion with Michal on IRC: scope export should only be occurring on the main thread.

The only new exports now are in the Unity.Internal namespace.

[Michal Hruby (mhr3)]

158 + public ScopeDBusNameManager ()

Singleton class, let's make this private.

164 + public static ScopeDBusNameManager get ()

The rest of libunity uses get_default () for singletons, let's keep it consistent.

225 + warning (@"Unable to own DBus name '$dbus_name'");
226 + Idle.add_full (Priority.DEFAULT, acquire_names.callback);

You can't resume the async method on first failure to own a name, if there are other own_name calls in progress, their callbacks would cause a crash.

review: Needs Fixing

[lp:~jamesh/libunity/export-multiple-scopes]

229. By James Henstridge on 2013-06-18

Rename ScopeDbusNameManager.get() to get_default(), and make constructor
private.

230. By James Henstridge on 2013-06-18

In acquire_names() wait for all names to be acquired or lost before
returning.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

LGTM :)

review: Approve

=== modified file 'debian/libunity9.symbols'
--- debian/libunity9.symbols	2013-06-10 18:06:29 +0000
+++ debian/libunity9.symbols	2013-06-18 05:48:29 +0000
@@ -355,6 +355,9 @@
  unity_internal_io_system_data_dirs_length1@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_merge_strategy_get_type@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_merge_strategy_merge_result@Base 7.0.0daily13.05.31ubuntu.unity.next
+ unity_internal_owned_name_dup@Base 0replaceme
+ unity_internal_owned_name_free@Base 0replaceme
+ unity_internal_owned_name_get_type@Base 0replaceme
  unity_internal_result_column_get_type@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_results_synchronizer_add_provider@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_results_synchronizer_clear@Base 7.0.0daily13.05.31ubuntu.unity.next
@@ -393,6 +396,12 @@
  unity_internal_scope_dbus_impl_set_categories_model@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_scope_dbus_impl_set_filters_model@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_scope_dbus_impl_unexport@Base 7.0.0daily13.05.31ubuntu.unity.next
+ unity_internal_scope_dbus_name_manager_acquire_names@Base 0replaceme
+ unity_internal_scope_dbus_name_manager_acquire_names_finish@Base 0replaceme
+ unity_internal_scope_dbus_name_manager_get_default@Base 0replaceme
+ unity_internal_scope_dbus_name_manager_get_type@Base 0replaceme
+ unity_internal_scope_dbus_name_manager_own_name@Base 0replaceme
+ unity_internal_scope_dbus_name_manager_unown_name@Base 0replaceme
  unity_internal_utils_async_mutex_construct@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_utils_async_mutex_get_type@Base 7.0.0daily13.05.31ubuntu.unity.next
  unity_internal_utils_async_mutex_lock@Base 7.0.0daily13.05.31ubuntu.unity.next

=== modified file 'src/unity-scope-dbus-connector.vala'
--- src/unity-scope-dbus-connector.vala	2013-03-28 13:34:16 +0000
+++ src/unity-scope-dbus-connector.vala	2013-06-18 05:48:29 +0000
@@ -40,7 +40,7 @@
 
   private Internal.DefaultScopeDBusImpl pimpl;
   private bool exported;
-  private uint bus_name_own_handle;
+  private bool name_owned;
 
   private static int proxy_usage_count = 0;
 
@@ -51,8 +51,9 @@
       if (pimpl == null) pimpl = new Internal.DefaultScopeDBusImpl (scope);
       pimpl.timeout = DAEMON_TIMEOUT_SEC;
       pimpl.on_timeout_reached.connect (timeout_reached);
-      own_name (); // this will also export the object
+      pimpl.export ();
       exported = true;
+      own_name ();
 
       proxy_usage_count++;
     }
@@ -81,59 +82,22 @@
     });
   }
 
-  private void own_name () throws Error
+  private void own_name ()
   {
+    var manager = Internal.ScopeDBusNameManager.get_default ();
     var dbus_name = scope.get_group_name ();
-    // check if the name is owned already
-    // FIXME: but track which names does this process own, cause trying to own
-    // an already owned name will fail (even though the caller is the owner)!
-    bool has_owner;
-    DBusConnection bus = Bus.get_sync (BusType.SESSION);
-    Variant result = bus.call_sync ("org.freedesktop.DBus",
-                                    "/org/freedesktop/dbus",
-                                    "org.freedesktop.DBus",
-                                    "NameHasOwner",
-                                    new Variant ("(s)", dbus_name),
-                                    new VariantType ("(b)"),
-                                    DBusCallFlags.NO_AUTO_START,
-                                    -1);
-    result.get ("(b)", out has_owner);
-    if (has_owner)
-    {
-      throw new IOError.FAILED ("Name '%s' is already owned".printf (dbus_name));
-    }
-
-    bool got_name = false;
-    var ml = new MainLoop ();
-    bus_name_own_handle = Bus.own_name (
-      BusType.SESSION, dbus_name, BusNameOwnerFlags.NONE, () =>
-      {
-        // connection acquired callback
-        try
-        {
-          pimpl.export ();
-        }
-        catch (Error err)
-        {
-          warning ("Unable to export object: %s", err.message);
-        }
-      },
-      () => { got_name = true; ml.quit (); },
-      () => { ml.quit (); }
-    );
-    ml.run ();
-    if (!got_name)
-    {
-      throw new IOError.FAILED ("Unable to own DBus name '%s'".printf (dbus_name));
-    }
+    manager.own_name (dbus_name);
+    name_owned = true;
   }
 
-  private void unown_name ()
+  private void unown_name()
   {
-    if (bus_name_own_handle != 0)
+    if (name_owned)
     {
-      Bus.unown_name (bus_name_own_handle);
-      bus_name_own_handle = 0;
+      var manager = Internal.ScopeDBusNameManager.get_default ();
+      var dbus_name = scope.get_group_name ();
+      manager.unown_name (dbus_name);
+      name_owned = false;
     }
   }
 
@@ -142,6 +106,17 @@
   public static void run ()
   {
     if (primary_loop == null) primary_loop = new MainLoop ();
+
+    var manager = Internal.ScopeDBusNameManager.get_default ();
+    manager.acquire_names.begin ((obj, result) =>
+      {
+        if (!manager.acquire_names.end (result))
+        {
+          warning ("Failed to acquire all required D-Bus names");
+          primary_loop.quit ();
+        }
+      });
+
     primary_loop.run ();
   }
 

=== modified file 'src/unity-scope-dbus-impl.vala'
--- src/unity-scope-dbus-impl.vala	2013-06-10 17:56:20 +0000
+++ src/unity-scope-dbus-impl.vala	2013-06-18 05:48:29 +0000
@@ -786,4 +786,97 @@
   }
 }
 
+private struct OwnedName {
+  public int ref_count;
+  public uint bus_name_own_handle;
+}
+
+private class ScopeDBusNameManager : Object
+{
+  private HashTable<string,OwnedName?> owned_names;
+
+  private ScopeDBusNameManager ()
+  {
+    owned_names = new HashTable<string,OwnedName?> (str_hash, str_equal);
+  }
+
+  static ScopeDBusNameManager? name_manager;
+  public static ScopeDBusNameManager get_default ()
+  {
+    if (name_manager == null)
+    {
+      name_manager = new ScopeDBusNameManager ();
+    }
+    return name_manager;
+  }
+
+  public void own_name (string dbus_name)
+  {
+    unowned OwnedName? info = owned_names.get (dbus_name);
+    if (info != null)
+    {
+      info.ref_count += 1;
+    }
+    else
+    {
+      owned_names.insert (dbus_name, {1, 0});
+    }
+  }
+
+  public void unown_name (string dbus_name)
+  {
+    unowned Internal.OwnedName? info = owned_names.get (dbus_name);
+    if (info != null)
+    {
+      info.ref_count -= 1;
+      // Ref count dropped to zero => release the name if it has been acquired
+      if (info.ref_count <= 0)
+      {
+        if (info.bus_name_own_handle != 0)
+        {
+          Bus.unown_name (info.bus_name_own_handle);
+        }
+        owned_names.remove(dbus_name);
+      }
+    }
+  }
+
+  public async bool acquire_names ()
+  {
+    var count = 0;
+    var failures = 0;
+    owned_names.for_each ((dbus_name, info) =>
+      {
+        if (info.bus_name_own_handle != 0)
+        {
+          // The name has already been requested.
+          return;
+        }
+        count += 1;
+        info.bus_name_own_handle = Bus.own_name (
+          BusType.SESSION, dbus_name, BusNameOwnerFlags.NONE, null,
+          () => {
+            count -= 1;
+            if (count <= 0)
+            {
+              Idle.add_full (Priority.DEFAULT, acquire_names.callback);
+            }
+          },
+          () => {
+            failures += 1;
+            warning (@"Unable to own DBus name '$dbus_name'");
+            count -= 1;
+            if (count <= 0)
+            {
+              Idle.add_full (Priority.DEFAULT, acquire_names.callback);
+            }
+          }
+          );
+      });
+    yield;
+    return failures == 0;
+  }
+
+}
+
 } /* namespace */

=== modified file 'test/vala/test-scope.vala'
--- test/vala/test-scope.vala	2013-05-30 23:42:30 +0000
+++ test/vala/test-scope.vala	2013-06-18 05:48:29 +0000
@@ -86,6 +86,8 @@
 
     Test.add_data_func ("/Unit/Scope/Export",
         Fixture.create<ScopeInitTester> (ScopeInitTester.test_scope_export));
+    Test.add_data_func ("/Unit/Scope/ExportTwoScopes",
+        Fixture.create<ScopeInitTester> (ScopeInitTester.test_scope_export_two_scopes));
     Test.add_data_func ("/Unit/Scope/ProxyConnection",
         Fixture.create<ScopeInitTester> (ScopeInitTester.test_scope_proxy));
     Test.add_data_func ("/Unit/Scope/OpenChannel",
@@ -204,11 +206,12 @@
 
   class TestScope: Unity.AbstractScope
   {
+    public string dbus_name { get; construct set; }
     public string dbus_path { get; construct set; }
 
-    public TestScope (string dbus_path)
+    public TestScope (string dbus_name, string dbus_path)
     {
-      Object (dbus_path: dbus_path);
+      Object (dbus_name: dbus_name, dbus_path: dbus_path);
     }
 
     public override Unity.ScopeSearchBase create_search_for_query (Unity.SearchContext ctx)
@@ -269,7 +272,7 @@
 
     public override string get_group_name ()
     {
-      return DBUS_NAME;
+      return dbus_name;
     }
 
     public override string get_unique_name ()
@@ -317,6 +320,20 @@
       return proxy;
     }
 
+    public static bool acquire_names ()
+    {
+      var ml = new MainLoop ();
+      var manager = Unity.Internal.ScopeDBusNameManager.get_default ();
+      bool success = false;
+      manager.acquire_names.begin ((obj, result) =>
+        {
+          success = manager.acquire_names.end (result);
+          ml.quit();
+        });
+      ml.run ();
+      return success;
+    }
+
     private void setup ()
     {
     }
@@ -328,22 +345,48 @@
 
     public void test_scope_export ()
     {
-      scope = new TestScope (DBUS_PATH);
-      try
-      {
-        scope.export ();
-      }
-      catch (Error err) { assert_not_reached (); }
+      scope = new TestScope (DBUS_NAME, DBUS_PATH);
+      try
+      {
+        scope.export ();
+      }
+      catch (Error err) { assert_not_reached (); }
+
+      assert (acquire_names ());
+    }
+
+    public void test_scope_export_two_scopes ()
+    {
+      scope = new TestScope (DBUS_NAME, DBUS_PATH);
+      try
+      {
+        scope.export ();
+      }
+      catch (Error err) { assert_not_reached (); }
+
+      // Export a second scope using the same D-Bus name
+      var scope2 = new TestScope (DBUS_NAME, "/second/scope/path");
+      try
+      {
+        scope2.export ();
+      }
+      catch (Error err) { assert_not_reached (); }
+      finally
+      {
+        scope2.unexport ();
+      }
+      assert (acquire_names ());
     }
 
     public void test_scope_proxy ()
     {
-      scope = new TestScope (DBUS_PATH);
+      scope = new TestScope (DBUS_NAME, DBUS_PATH);
       try
       {
         scope.export ();
       }
       catch (Error err) { assert_not_reached (); }
+      assert (acquire_names ());
 
       var proxy = acquire_test_proxy ();
       assert (proxy != null);
@@ -363,13 +406,14 @@
     {
       base.setup ();
 
-      scope = new TestScope (DBUS_PATH);
-      
+      scope = new TestScope (DBUS_NAME, DBUS_PATH);
+
       try
       {
         scope.export ();
       }
       catch (Error err) { assert_not_reached (); }
+      assert (acquire_names ());
 
       proxy = ScopeInitTester.acquire_test_proxy ();
       assert (proxy != null);