D-Bus server + GIO D-Bus: fix auto-activation (Debian bug #599247)

When syncevo-dbus-server was started on demand by the D-Bus daemon,
then it registered itself with the daemon before it was ready to
serve requests. Only happened in combination with GIO D-Bus and
thus was not a problem before 1.2.99.x.

One user-visible effect was that the GTK UI did select the default
service when it was started for the first time, because it could not
retrieve that information from syncevo-dbus-server.

The fix consists of delaying the name acquisition. That gives the
caller a chance to register D-Bus objects first, before completing the
connection setup. The semantic change is that
dbus_bus_connection_undelay() must be called on new connections which
have a name (a NOP with libdbus).

This patch tries to minimize code changes. The downside of not
changing the GDBusCXX API more radically is that the bus name must be
attached to DBusConnectionPtr, where it will be copied into each
reference to the connection. Hopefully std::string is smart enough to
share the (small) data in this case. Should be solved cleaner once
libdbus support can be deprecated.

A test for auto-activation and double start of syncevo-dbus-server
is also added.

src/dbus/server/main.cpp

@@ -150,6 +150,7 @@ int main(int argc, char **argv, char **envp)
             unsetenv("G_DBUS_DEBUG");
         }
 
+        dbus_bus_connection_undelay(conn);
         server->run();
         SE_LOG_DEBUG(NULL, NULL, "cleaning up");
         server.reset();

src/gdbusxx/gdbus-cxx-bridge.cpp

@@ -33,12 +33,63 @@ namespace GDBusCXX {
 MethodHandler::MethodMap MethodHandler::m_methodMap;
 boost::function<void (void)> MethodHandler::m_callback;
 
-static void GDBusNameLost(GDBusConnection *connection,
-                          const gchar *name,
-                          gpointer user_data)
+// It should be okay to use global variables here because they are
+// only used inside the main thread while it waits in undelay() for a
+// positive or negative result to g_bus_own_name_on_connection().
+// Once acquired, the name can only get lost again when the
+// D-Bus daemon dies (no name owership alloed), in which case the
+// process dies anyway.
+static bool nameError;
+static bool nameAcquired;
+static void BusNameAcquired(GDBusConnection *connection,
+                            const gchar *name,
+                            gpointer userData) throw ()
 {
-    g_critical("lost D-Bus connection or failed to obtain %s D-Bus name, quitting", name);
-    exit(1);
+    try {
+        g_debug("got D-Bus name %s", name);
+        nameAcquired = true;
+    } catch (...) {
+        nameError = true;
+    }
+}
+
+static void BusNameLost(GDBusConnection *connection,
+                        const gchar *name,
+                        gpointer userData) throw ()
+{
+    try {
+        g_debug("lost %s %s",
+                connection ? "D-Bus connection for name" :
+                "D-Bus name",
+                name);
+    } catch (...) {
+    }
+    nameError = true;
+}
+
+void DBusConnectionPtr::undelay() const
+{
+    if (!m_name.empty()) {
+        g_debug("starting to acquire D-Bus name %s", m_name.c_str());
+        nameAcquired = false;
+        nameError = false;
+        char *copy = g_strdup(m_name.c_str());
+        g_bus_own_name_on_connection(get(),
+                                     copy,
+                                     G_BUS_NAME_OWNER_FLAGS_NONE,
+                                     BusNameAcquired,
+                                     BusNameLost,
+                                     copy,
+                                     g_free);
+        while (!nameAcquired && !nameError) {
+            g_main_context_iteration(NULL, true);
+        }
+        g_debug("done with acquisition of %s", m_name.c_str());
+        if (nameError) {
+            throw std::runtime_error("could not obtain D-Bus name - already running?");
+        }
+    }
+    g_dbus_connection_start_message_processing(get());
 }
 
 DBusConnectionPtr dbus_get_bus_connection(const char *busType,
@@ -48,10 +99,13 @@ DBusConnectionPtr dbus_get_bus_connection(const char *busType,
 {
     DBusConnectionPtr conn;
     GError* error = NULL;
+    GBusType type =
+        boost::iequals(busType, "SESSION") ?
+        G_BUS_TYPE_SESSION :
+        G_BUS_TYPE_SYSTEM;
 
-    if(unshared) {
-        char *address = g_dbus_address_get_for_bus_sync(boost::iequals(busType, "SESSION") ?
-                                                        G_BUS_TYPE_SESSION : G_BUS_TYPE_SYSTEM,
+    if (unshared) {
+        char *address = g_dbus_address_get_for_bus_sync(type,
                                                         NULL, &error);
         if(address == NULL) {
             if (err) {
@@ -76,9 +130,7 @@ DBusConnectionPtr dbus_get_bus_connection(const char *busType,
         }
     } else {
         // This returns a singleton, shared connection object.
-        conn = DBusConnectionPtr(g_bus_get_sync(boost::iequals(busType, "SESSION") ?
-                                                G_BUS_TYPE_SESSION :
-                                                G_BUS_TYPE_SYSTEM,
+        conn = DBusConnectionPtr(g_bus_get_sync(type,
                                                 NULL, &error),
                                  false);
         if(conn == NULL) {
@@ -89,11 +141,11 @@ DBusConnectionPtr dbus_get_bus_connection(const char *busType,
         }
     }
 
-    if(name) {
-        // Copy name, to ensure that it remains available.
-        char *copy = g_strdup(name);
-        g_bus_own_name_on_connection(conn.get(), copy, G_BUS_NAME_OWNER_FLAGS_NONE,
-                                     NULL, GDBusNameLost, copy, g_free);
+    if (name) {
+        // Request name later in undelay(), after the caller
+        // had a chance to add objects.
+        conn.addName(name);
+        // Acting as client, need to stop when D-Bus daemon dies.
         g_dbus_connection_set_exit_on_close(conn.get(), TRUE);
     }
 
@@ -124,11 +176,6 @@ DBusConnectionPtr dbus_get_bus_connection(const std::string &address,
     return conn;
 }
 
-void dbus_bus_connection_undelay(const DBusConnectionPtr &conn)
-{
-    g_dbus_connection_start_message_processing(conn.get());
-}
-
 static void ConnectionLost(GDBusConnection *connection,
                            gboolean remotePeerVanished,
                            GError *error,

src/gdbusxx/gdbus-cxx-bridge.h

@@ -124,6 +124,14 @@ inline void throwFailure(const std::string &object,
 
 class DBusConnectionPtr : public boost::intrusive_ptr<GDBusConnection>
 {
+    /**
+     * Bus name of client, as passed to dbus_get_bus_connection().
+     * The name will be requested in dbus_bus_connection_undelay() =
+     * undelay(), to give the caller a chance to register objects on
+     * the new connection.
+     */
+    std::string m_name;
+
  public:
     DBusConnectionPtr() {}
     // connections are typically created once, so increment the ref counter by default
@@ -141,6 +149,9 @@ class DBusConnectionPtr : public boost::intrusive_ptr<GDBusConnection>
     typedef boost::function<void ()> Disconnect_t;
     void setDisconnect(const Disconnect_t &func);
     // #define GDBUS_CXX_HAVE_DISCONNECT 1
+
+    void undelay() const;
+    void addName(const std::string &name) { m_name = name; }
 };
 
 class DBusMessagePtr : public boost::intrusive_ptr<GDBusMessage>
@@ -224,7 +235,7 @@ DBusConnectionPtr dbus_get_bus_connection(const std::string &address,
                                           DBusErrorCXX *err,
                                           bool delayed = false);
 
-void dbus_bus_connection_undelay(const DBusConnectionPtr &conn);
+inline void dbus_bus_connection_undelay(const DBusConnectionPtr &conn) { conn.undelay(); }
 
 /**
  * Wrapper around DBusServer. Does intentionally not expose

test/test-dbus.py

@@ -63,6 +63,18 @@ configName = "dbus_unittest"
 def usingValgrind():
     return 'valgrind' in os.environ.get("TEST_DBUS_PREFIX", "")
 
+def which(program):
+    '''find absolute path to program (simple file name, no path) in PATH env variable'''
+    def isExe(fpath):
+        return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
+
+    for path in os.environ["PATH"].split(os.pathsep):
+        exeFile = os.path.join(path, program)
+        if isExe(exeFile):
+            return os.path.abspath(exeFile)
+
+    return None
+
 def GrepNotifications(dbuslog):
     '''finds all Notify calls and returns their parameters as list of line lists'''
     return re.findall(r'^method call .* dest=.* .*interface=org.freedesktop.Notifications; member=Notify\n((?:^   .*\n)*)',
@@ -1250,6 +1262,56 @@ class TestDBusServer(DBusUtil, unittest.TestCase):
         else:
             self.fail("no exception thrown")
 
+class TestDBusServerStart(DBusUtil, unittest.TestCase):
+    def testAutoActivation(self):
+        '''TestDBusServerStart.testAutoActivation - check that activating syncevo-dbus-server via D-Bus daemon works'''
+        # Create a D-Bus service file for the syncevo-dbus-server that we
+        # are testing (= the one in PATH).
+        shutil.rmtree(xdg_root, True)
+        dirname = os.path.join(xdg_root, "share", "dbus-1", "services")
+        os.makedirs(dirname)
+        service = open(os.path.join(dirname, "org.syncevolution.service"), "w")
+        service.write('''[D-BUS Service]
+Name=org.syncevolution
+Exec=%s
+''' % which('syncevo-dbus-server'))
+        service.close()
+
+        # Now run a private D-Bus session in which dbus-send activates
+        # that syncevo-dbus-server. Uses a dbus-session.sh from the
+        # same dir as test-dbus.py itself.
+        env = copy.deepcopy(os.environ)
+        env['XDG_DATA_DIRS'] = os.path.abspath(os.path.join(xdg_root, "share"))
+
+        # First run something which just starts the daemon.
+        dbus = subprocess.Popen((os.path.join(os.path.dirname(sys.argv[0]), 'dbus-session.sh'),
+                                 'dbus-send',
+                                 '--print-reply',
+                                 '--dest=org.syncevolution',
+                                 '/',
+                                 'org.freedesktop.DBus.Introspectable.Introspect'),
+                                env=env,
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.STDOUT)
+        (out, err) = dbus.communicate()
+        self.assertEqual(0, dbus.returncode,
+                         msg='introspection of syncevo-dbus-server failed:\n' + out)
+
+        # Now try some real command.
+        dbus = subprocess.Popen((os.path.join(os.path.dirname(sys.argv[0]), 'dbus-session.sh'),
+                                 'dbus-send',
+                                 '--print-reply',
+                                 '--dest=org.syncevolution',
+                                 '/org/syncevolution/Server',
+                                 'org.syncevolution.Server.GetVersions'),
+                                env=env,
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.STDOUT)
+        (out, err) = dbus.communicate()
+        self.assertEqual(0, dbus.returncode,
+                         msg='GetVersions of syncevo-dbus-server failed:\n' + out)
+
+
 class TestDBusServerTerm(DBusUtil, unittest.TestCase):
     def setUp(self):
         self.setUpServer()
@@ -1257,6 +1319,15 @@ class TestDBusServerTerm(DBusUtil, unittest.TestCase):
     def run(self, result):
         self.runTest(result, True, ["-d", "10"])
 
+    def testSingleton(self):
+        """TestDBusServerTerm.testSingleton - a second instance of syncevo-dbus-server must terminate right away"""
+        dbus = subprocess.Popen([ 'syncevo-dbus-server' ],
+                                stdout=subprocess.PIPE,
+                                stderr=subprocess.STDOUT)
+        (out, err) = dbus.communicate()
+        if not re.search(r'''\[ERROR.*already running\?\n''', out):
+            self.fail('second dbus server did not recognize already running server:\n%s' % out)
+
     @timeout(100)
     def testNoTerm(self):
         """TestDBusServerTerm.testNoTerm - D-Bus server must stay around during calls"""