Merge lp:~gerboland/unity8/async-dbus-dashcomm into lp:unity8

[Gerry Boland]

DashCommunicator: replace QDBusInterface with a non-blocking simplification which does not introspect the service on creation

QDBusInterface on creation introspects the service it is pointed to. This introspection is performed on the GUI thread, no matter what thread QDBusInterface is created on - see the moveToThread in qtbase/src/dbus/qdbusconnection.cpp.

Therefore should the service being introspected is busy at the time, the GUI thread is blocked, which is a bad thing. This isn't too easily noticed today with unity8 on startup, but it is there (Greeter not reacting quickly).

It is much easier to notice if you ssh into the phone and do "stop unity8-dash; start unity-dash" and just watch the spinner as dash is starting. It can be seen to hang for 500ms+ - that's the entire shell GUI blocked for over 1/2 a second.

What do we loose not using QDbusInterface directly? Mainly the ability to connect to dbus signals from the service. QDbusInterface creates a meta object which represents the service's interface, and translates meta object calls to dbus calls. As DashCommunicator just calls dbus methods, this can be handled without any meta object bits, so the QDBusAbstractInterface has enough to do this.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
N
 * Did you perform an exploratory manual test run of your code change and any related functionality?
Y on N7 & desktop
 * Did you make sure that your branch does not contain spurious tags?
Y
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A
 * If you changed the UI, has there been a design review?
N/A

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1522
http://jenkins.qa.ubuntu.com/job/unity8-ci/5059/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/614
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/223
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/224
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/224
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/527
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/612
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/612/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16665

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5059/rebuild

review: Approve (continuous-integration)

[Lorn Potter (lorn-potter)]

This change looks ok to me.

[Michał Sawicz (saviq)]

Typo.

[lp:~gerboland/unity8/async-dbus-dashcomm]

1523. By Gerry Boland on 2015-01-05

Fix typo

[Gerry Boland (gerboland)]

Typo fixed

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1523
http://jenkins.qa.ubuntu.com/job/unity8-ci/5072/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/644
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/236
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/553
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/642
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/642/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16795

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5072/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1523
http://jenkins.qa.ubuntu.com/job/unity8-ci/5086/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/686
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/250
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/251
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/251
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/591
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/684
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/684/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16872

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5086/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1523
http://jenkins.qa.ubuntu.com/job/unity8-ci/5125/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/745
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/289
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/290
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/290
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/647
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/743
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/743/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16955

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5125/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1523
http://jenkins.qa.ubuntu.com/job/unity8-ci/5136/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/768
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/668
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/766
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/766/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/16996

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5136/rebuild

review: Needs Fixing (continuous-integration)

[Michael Zanetti (mzanetti)]

 * Did you perform an exploratory manual test run of the code change and any related functionality?

yes

 * Did CI run pass? If not, please explain why.

no, but it's a known failure in trunk

 * Did you make sure that the branch does not contain spurious tags?

yes

review: Approve

=== modified file 'plugins/Unity/DashCommunicator/dashconnection.cpp'
--- plugins/Unity/DashCommunicator/dashconnection.cpp	2014-10-20 15:52:56 +0000
+++ plugins/Unity/DashCommunicator/dashconnection.cpp	2015-01-05 11:13:21 +0000
@@ -19,12 +19,42 @@
 #include <QDBusInterface>
 #include <QDBusPendingCall>
 
+/* The default implementation of AbstractDBusServiceMonitor creates a QDBusInterface when the service
+ * appears on the bus.
+ *
+ * On construction QDBusInterface synchronously introspects the service, which will block the GUI
+ * thread of this process if the service is busy. QDBusAbstractInterface does not perform this
+ * introspection, so let's subclass that and avoid the blocking scenario.
+ *
+ * However we lose Qt's wrapping of the DBus service with a MetaObject, with the result that we
+ * cannot easily connect to DBus signals with the usual connect() calls. So this approach only
+ * suited to push communication with the DBus service.
+ */
+class AsyncDBusInterface : public QDBusAbstractInterface
+{
+public:
+    AsyncDBusInterface(const QString &service, const QString &path,
+                       const QString &interface, const QDBusConnection &connection,
+                       QObject *parent = 0)
+    : QDBusAbstractInterface(service, path, interface.toLatin1().data(), connection, parent)
+    {}
+    ~AsyncDBusInterface() = default;
+};
+
+
 DashConnection::DashConnection(const QString &service, const QString &path, const QString &interface, QObject *parent):
     AbstractDBusServiceMonitor(service, path, interface, SessionBus, parent)
 {
 
 }
 
+/* Override the default implementation to create a non-blocking DBus interface (see note above). */
+QDBusAbstractInterface* DashConnection::createInterface(const QString &service, const QString &path,
+                                        const QString &interface, const QDBusConnection &connection)
+{
+    return new AsyncDBusInterface(service, path, interface, connection);
+}
+
 void DashConnection::setCurrentScope(int index, bool animate, bool isSwipe)
 {
     if (dbusInterface()) {

=== modified file 'plugins/Unity/DashCommunicator/dashconnection.h'
--- plugins/Unity/DashCommunicator/dashconnection.h	2014-10-20 15:52:56 +0000
+++ plugins/Unity/DashCommunicator/dashconnection.h	2015-01-05 11:13:21 +0000
@@ -20,7 +20,6 @@
 // local
 #include "abstractdbusservicemonitor.h"
 
-
 class DashConnection: public AbstractDBusServiceMonitor
 {
     Q_OBJECT
@@ -29,6 +28,10 @@
 
 public Q_SLOTS:
     void setCurrentScope(int index, bool animate, bool isSwipe);
+
+private:
+    QDBusAbstractInterface* createInterface(const QString &service, const QString &path,
+                                            const QString &interface, const QDBusConnection &connection) override;
 };
 
 #endif

=== modified file 'src/libunity8-private/abstractdbusservicemonitor.cpp'
--- src/libunity8-private/abstractdbusservicemonitor.cpp	2014-09-29 09:56:41 +0000
+++ src/libunity8-private/abstractdbusservicemonitor.cpp	2015-01-05 11:13:21 +0000
@@ -32,19 +32,20 @@
     , m_service(service)
     , m_path(path)
     , m_interface(interface)
+    , m_bus(bus)
     , m_watcher(new QDBusServiceWatcher(service,
                                         (bus == SystemBus) ? QDBusConnection::systemBus()
                                                            : QDBusConnection::sessionBus()))
     , m_dbusInterface(nullptr)
 {
-    connect(m_watcher, &QDBusServiceWatcher::serviceRegistered, this, &AbstractDBusServiceMonitor::createInterface);
-    connect(m_watcher, &QDBusServiceWatcher::serviceUnregistered, this, &AbstractDBusServiceMonitor::destroyInterface);
+    connect(m_watcher, &QDBusServiceWatcher::serviceRegistered, this, &AbstractDBusServiceMonitor::onServiceRegistered);
+    connect(m_watcher, &QDBusServiceWatcher::serviceUnregistered, this, &AbstractDBusServiceMonitor::onServiceUnregistered);
 
     // Connect to the service if it's up already
     QDBusConnectionInterface* sessionBus = QDBusConnection::sessionBus().interface();
     QDBusReply<bool> reply = sessionBus->isServiceRegistered(m_service);
     if (reply.isValid() && reply.value()) {
-        createInterface(m_service);
+        onServiceRegistered(m_service);
     }
 }
 
@@ -54,19 +55,31 @@
     delete m_dbusInterface;
 }
 
-void AbstractDBusServiceMonitor::createInterface(const QString &)
+void AbstractDBusServiceMonitor::onServiceRegistered(const QString &)
 {
     if (m_dbusInterface != nullptr) {
         delete m_dbusInterface;
         m_dbusInterface = nullptr;
     }
 
-    m_dbusInterface = new QDBusInterface(m_service, m_path, m_interface,
-                                         QDBusConnection::sessionBus());
+    m_dbusInterface = createInterface(m_service, m_path, m_interface,
+                                      (m_bus == SystemBus) ? QDBusConnection::systemBus()
+                                                           : QDBusConnection::sessionBus());
     Q_EMIT serviceAvailableChanged(true);
 }
 
-void AbstractDBusServiceMonitor::destroyInterface(const QString &)
+/*
+ * Default implementation creates a QDBusInterface. This performs blocking introspection of the
+ * service at initialization, which may be undesirable if the service is slow/blocked.
+ */
+QDBusAbstractInterface*
+AbstractDBusServiceMonitor::createInterface(const QString &service, const QString &path,
+                                            const QString &interface, const QDBusConnection &connection)
+{
+    return new QDBusInterface(service, path, interface, connection);
+}
+
+void AbstractDBusServiceMonitor::onServiceUnregistered(const QString &)
 {
     if (m_dbusInterface != nullptr) {
         delete m_dbusInterface;
@@ -76,7 +89,7 @@
     Q_EMIT serviceAvailableChanged(false);
 }
 
-QDBusInterface* AbstractDBusServiceMonitor::dbusInterface() const
+QDBusAbstractInterface* AbstractDBusServiceMonitor::dbusInterface() const
 {
     return m_dbusInterface;
 }

=== modified file 'src/libunity8-private/abstractdbusservicemonitor.h'
--- src/libunity8-private/abstractdbusservicemonitor.h	2014-09-29 09:43:18 +0000
+++ src/libunity8-private/abstractdbusservicemonitor.h	2015-01-05 11:13:21 +0000
@@ -24,7 +24,7 @@
 #include <QString>
 #include <QDBusConnection>
 
-class QDBusInterface;
+class QDBusAbstractInterface;
 class QDBusServiceWatcher;
 
 class Q_DECL_EXPORT AbstractDBusServiceMonitor : public QObject
@@ -44,7 +44,7 @@
                                         QObject *parent = 0);
     ~AbstractDBusServiceMonitor();
 
-    QDBusInterface* dbusInterface() const;
+    QDBusAbstractInterface* dbusInterface() const;
 
     bool serviceAvailable() const;
 
@@ -52,15 +52,19 @@
     void serviceAvailableChanged(bool available);
 
 private Q_SLOTS:
-    void createInterface(const QString &service);
-    void destroyInterface(const QString &service);
+    void onServiceRegistered(const QString &service);
+    void onServiceUnregistered(const QString &service);
 
 protected:
+    virtual QDBusAbstractInterface* createInterface(const QString &service, const QString &path,
+                                                    const QString &interface, const QDBusConnection &connection);
+
     const QString m_service;
     const QString m_path;
     const QString m_interface;
+    const Bus m_bus;
     QDBusServiceWatcher* m_watcher;
-    QDBusInterface* m_dbusInterface;
+    QDBusAbstractInterface* m_dbusInterface;
 };
 
 #endif // ABSTRACTDBUSSERVICEMONITOR_H