From c2049f67e4cfe5f09e1b033b910cb37d043a287e Mon Sep 17 00:00:00 2001
From: Thiago Macieira <thiago.macieira@intel.com>
Date: Mon, 29 Dec 2014 11:35:36 -0200
Subject: [PATCH] Use a dedicated thread for handling incoming libdbus-1 events
Each application will have one thread dedicated for this, for all
QDBusConnections. I wouldn't mind sharing such a thread with other uses
in Qt, provided none of them ever block (the QProcessManager thread
comes to mind, but it's going away soon).
The cost associated with this change in this commit is so far rather
minimal. All incoming D-Bus calls need to be handled after an event is
posted anyway, to avoid deadlocking on reentering libdbus-1 functions
that acquire locks still held. The cost is the one more thread running
and the cost of synchronizing them when an event is posted.
The benefits far outweigh that cost: no longer will we have problems of
QtDBus failing to run if the main system or session connections are used
before QCoreApplication is run. Moreover, events can be received and
handled in aux threads even if the main thread is blocked on some
operation.
Note: this commit may not be testable (tst_qdbusconnection may fail)
Task-number: QTBUG-43585
Change-Id: Ic5d393bfd36e48a193fcffff13b737556ccd11a8
Reviewed-by: Albert Astals Cid <aacid@kde.org>
Reviewed-by: Alex Blasche <alexander.blasche@theqtcompany.com>
---
src/dbus/qdbusconnection.cpp | 58 +++++++++++--------
src/dbus/qdbusconnectionmanager_p.h | 11 +++-
src/dbus/qdbusintegrator.cpp | 12 +---
src/dbus/qdbusserver.cpp | 6 +-
.../qdbuspendingcall/tst_qdbuspendingcall.cpp | 33 +----------
5 files changed, 50 insertions(+), 70 deletions(-)
diff --git a/src/dbus/qdbusconnection.cpp b/src/dbus/qdbusconnection.cpp
index 08a21c5d026..47255992ae4 100644
--- a/src/dbus/qdbusconnection.cpp
+++ b/src/dbus/qdbusconnection.cpp
@@ -1,6 +1,7 @@
/****************************************************************************
**
** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
** Contact: http://www.qt.io/licensing/
**
** This file is part of the QtDBus module of the Qt Toolkit.
@@ -77,17 +78,16 @@ void QDBusConnectionManager::removeConnection(const QString &name)
// ### Output a warning if connections are being used after they have been removed.
}
+QDBusConnectionManager::QDBusConnectionManager()
+{
+ moveToThread(Q_NULLPTR); // we don't handle events
+ start();
+}
+
QDBusConnectionManager::~QDBusConnectionManager()
{
- for (QHash<QString, QDBusConnectionPrivate *>::const_iterator it = connectionHash.constBegin();
- it != connectionHash.constEnd(); ++it) {
- QDBusConnectionPrivate *d = it.value();
- if (!d->ref.deref())
- d->deleteYourself();
- else
- d->closeConnection();
- }
- connectionHash.clear();
+ quit();
+ wait();
}
QDBusConnectionManager* QDBusConnectionManager::instance()
@@ -106,6 +106,25 @@ void QDBusConnectionManager::setConnection(const QString &name, QDBusConnectionP
c->name = name;
}
+void QDBusConnectionManager::run()
+{
+ exec();
+
+ // cleanup:
+ QMutexLocker locker(&mutex);
+ for (QHash<QString, QDBusConnectionPrivate *>::const_iterator it = connectionHash.constBegin();
+ it != connectionHash.constEnd(); ++it) {
+ QDBusConnectionPrivate *d = it.value();
+ if (!d->ref.deref()) {
+ delete d;
+ } else {
+ d->closeConnection();
+ d->moveToThread(Q_NULLPTR); // allow it to be deleted in another thread
+ }
+ }
+ connectionHash.clear();
+}
+
/*!
\class QDBusConnection
\inmodule QtDBus
@@ -295,8 +314,6 @@ QDBusConnection &QDBusConnection::operator=(const QDBusConnection &other)
*/
QDBusConnection QDBusConnection::connectToBus(BusType type, const QString &name)
{
-// Q_ASSERT_X(QCoreApplication::instance(), "QDBusConnection::addConnection",
-// "Cannot create connection without a Q[Core]Application instance");
if (!qdbus_loadLibDBus()) {
QDBusConnectionPrivate *d = 0;
return QDBusConnection(d);
@@ -331,6 +348,7 @@ QDBusConnection QDBusConnection::connectToBus(BusType type, const QString &name)
// create the bus service
// will lock in QDBusConnectionPrivate::connectRelay()
d->setBusService(retval);
+ d->moveToThread(_q_manager());
return retval;
}
@@ -342,8 +360,6 @@ QDBusConnection QDBusConnection::connectToBus(BusType type, const QString &name)
QDBusConnection QDBusConnection::connectToBus(const QString &address,
const QString &name)
{
-// Q_ASSERT_X(QCoreApplication::instance(), "QDBusConnection::addConnection",
-// "Cannot create connection without a Q[Core]Application instance");
if (!qdbus_loadLibDBus()) {
QDBusConnectionPrivate *d = 0;
return QDBusConnection(d);
@@ -373,6 +389,7 @@ QDBusConnection QDBusConnection::connectToBus(const QString &address,
// create the bus service
// will lock in QDBusConnectionPrivate::connectRelay()
d->setBusService(retval);
+ d->moveToThread(_q_manager());
return retval;
}
@@ -385,8 +402,6 @@ QDBusConnection QDBusConnection::connectToBus(const QString &address,
QDBusConnection QDBusConnection::connectToPeer(const QString &address,
const QString &name)
{
-// Q_ASSERT_X(QCoreApplication::instance(), "QDBusConnection::addConnection",
-// "Cannot create connection without a Q[Core]Application instance");
if (!qdbus_loadLibDBus()) {
QDBusConnectionPrivate *d = 0;
return QDBusConnection(d);
@@ -405,6 +420,7 @@ QDBusConnection QDBusConnection::connectToPeer(const QString &address,
d->setPeer(c, error);
_q_manager()->setConnection(name, d);
+ d->moveToThread(_q_manager());
QDBusConnection retval(d);
@@ -1061,16 +1077,7 @@ class QDBusDefaultConnection: public QDBusConnection
public:
inline QDBusDefaultConnection(BusType type, const char *name)
: QDBusConnection(connectToBus(type, QString::fromLatin1(name))), ownName(name)
- {
- // make sure this connection is running on the main thread
- QCoreApplication *instance = QCoreApplication::instance();
- if (!instance) {
- qWarning("QDBusConnection: %s D-Bus connection created before QCoreApplication. Application may misbehave.",
- type == SessionBus ? "session" : type == SystemBus ? "system" : "generic");
- } else if (QDBusConnectionPrivate::d(*this)) {
- QDBusConnectionPrivate::d(*this)->moveToThread(instance->thread());
- }
- }
+ { }
inline ~QDBusDefaultConnection()
{ disconnectFromBus(QString::fromLatin1(ownName)); }
@@ -1125,6 +1132,7 @@ QDBusConnection QDBusConnection::sender()
*/
void QDBusConnectionPrivate::setBusService(const QDBusConnection &connection)
{
+ Q_ASSERT(mode == ClientMode);
busService = new QDBusConnectionInterface(connection, this);
ref.deref(); // busService has increased the refcounting to us
// avoid cyclic refcounting
diff --git a/src/dbus/qdbusconnectionmanager_p.h b/src/dbus/qdbusconnectionmanager_p.h
index fc0bb515e59..c3c7999699d 100644
--- a/src/dbus/qdbusconnectionmanager_p.h
+++ b/src/dbus/qdbusconnectionmanager_p.h
@@ -1,6 +1,7 @@
/****************************************************************************
**
** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
** Contact: http://www.qt.io/licensing/
**
** This file is part of the QtDBus module of the Qt Toolkit.
@@ -47,15 +48,17 @@
#define QDBUSCONNECTIONMANAGER_P_H
#include "qdbusconnection_p.h"
+#include "private/qthread_p.h"
#ifndef QT_NO_DBUS
QT_BEGIN_NAMESPACE
-class QDBusConnectionManager
+class QDBusConnectionManager : public QDaemonThread
{
+ Q_OBJECT
public:
- QDBusConnectionManager() {}
+ QDBusConnectionManager();
~QDBusConnectionManager();
static QDBusConnectionManager* instance();
@@ -64,6 +67,10 @@ public:
void setConnection(const QString &name, QDBusConnectionPrivate *c);
mutable QMutex mutex;
+
+protected:
+ void run() Q_DECL_OVERRIDE;
+
private:
QHash<QString, QDBusConnectionPrivate *> connectionHash;
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 4ae6a7f351f..6e37ea6f7cf 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1091,13 +1091,7 @@ void QDBusConnectionPrivate::closeConnection()
void QDBusConnectionPrivate::checkThread()
{
- if (!thread()) {
- if (QCoreApplication::instance())
- moveToThread(QCoreApplication::instance()->thread());
- else
- qWarning("The thread that had QDBusConnection('%s') has died and there is no main thread",
- qPrintable(name));
- }
+ Q_ASSERT(thread() == QDBusConnectionManager::instance());
}
bool QDBusConnectionPrivate::handleError(const QDBusErrorInternal &error)
@@ -1833,12 +1827,12 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
call->pending = 0;
}
- locker.unlock();
-
// Are there any watchers?
if (call->watcherHelper)
call->watcherHelper->emitSignals(msg, call->sentMessage);
+ locker.unlock();
+
if (msg.type() == QDBusMessage::ErrorMessage)
emit connection->callWithCallbackFailed(QDBusError(msg), call->sentMessage);
diff --git a/src/dbus/qdbusserver.cpp b/src/dbus/qdbusserver.cpp
index 05156c992fb..a0b2c8283cf 100644
--- a/src/dbus/qdbusserver.cpp
+++ b/src/dbus/qdbusserver.cpp
@@ -63,13 +63,14 @@ QDBusServer::QDBusServer(const QString &address, QObject *parent)
d = 0;
return;
}
- d = new QDBusConnectionPrivate(this);
+ d = new QDBusConnectionPrivate;
QObject::connect(d, SIGNAL(newServerConnection(QDBusConnectionPrivate*)),
this, SLOT(_q_newConnection(QDBusConnectionPrivate*)), Qt::QueuedConnection);
QDBusErrorInternal error;
d->setServer(this, q_dbus_server_listen(address.toUtf8().constData(), error), error);
+ d->moveToThread(QDBusConnectionManager::instance());
}
/*!
@@ -91,13 +92,14 @@ QDBusServer::QDBusServer(QObject *parent)
d = 0;
return;
}
- d = new QDBusConnectionPrivate(this);
+ d = new QDBusConnectionPrivate;
QObject::connect(d, SIGNAL(newServerConnection(QDBusConnectionPrivate*)),
this, SLOT(_q_newConnection(QDBusConnectionPrivate*)), Qt::QueuedConnection);
QDBusErrorInternal error;
d->setServer(this, q_dbus_server_listen(address, error), error);
+ d->moveToThread(QDBusConnectionManager::instance());
}
/*!
diff --git a/tests/auto/dbus/qdbuspendingcall/tst_qdbuspendingcall.cpp b/tests/auto/dbus/qdbuspendingcall/tst_qdbuspendingcall.cpp
index 9c334f77a2a..cc12ef8bcca 100644
--- a/tests/auto/dbus/qdbuspendingcall/tst_qdbuspendingcall.cpp
+++ b/tests/auto/dbus/qdbuspendingcall/tst_qdbuspendingcall.cpp
@@ -1,6 +1,7 @@
/****************************************************************************
**
** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2015 Intel Corporation.
** Contact: http://www.qt.io/licensing/
**
** This file is part of the test suite of the Qt Toolkit.
@@ -172,10 +173,6 @@ QDBusPendingCall tst_QDBusPendingCall::sendError()
void tst_QDBusPendingCall::waitForFinished()
{
QDBusPendingCall ac = sendMessage();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
ac.waitForFinished();
QVERIFY(ac.isFinished());
QVERIFY(!ac.isError());
@@ -195,10 +192,6 @@ void tst_QDBusPendingCall::waitForFinished()
void tst_QDBusPendingCall::waitForFinished_error()
{
QDBusPendingCall ac = sendError();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
ac.waitForFinished();
QVERIFY(ac.isFinished());
QVERIFY(ac.isError());
@@ -254,10 +247,6 @@ void tst_QDBusPendingCall::callWithCallback_localLoop_errorReply()
void tst_QDBusPendingCall::watcher()
{
QDBusPendingCall ac = sendMessage();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
callCount = 0;
watchArgument = 0;
@@ -284,10 +273,6 @@ void tst_QDBusPendingCall::watcher()
void tst_QDBusPendingCall::watcher_error()
{
QDBusPendingCall ac = sendError();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
callCount = 0;
watchArgument = 0;
@@ -312,10 +297,6 @@ void tst_QDBusPendingCall::watcher_error()
void tst_QDBusPendingCall::watcher_waitForFinished()
{
QDBusPendingCall ac = sendMessage();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
callCount = 0;
watchArgument = 0;
@@ -391,10 +372,6 @@ void tst_QDBusPendingCall::watcher_waitForFinished_threaded()
void tst_QDBusPendingCall::watcher_waitForFinished_alreadyFinished()
{
QDBusPendingCall ac = sendMessage();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
ac.waitForFinished();
QVERIFY(ac.isFinished());
QVERIFY(!ac.isError());
@@ -425,10 +402,6 @@ void tst_QDBusPendingCall::watcher_waitForFinished_alreadyFinished()
void tst_QDBusPendingCall::watcher_waitForFinished_alreadyFinished_eventLoop()
{
QDBusPendingCall ac = sendMessage();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
ac.waitForFinished();
QVERIFY(ac.isFinished());
QVERIFY(!ac.isError());
@@ -462,10 +435,6 @@ void tst_QDBusPendingCall::watcher_waitForFinished_alreadyFinished_eventLoop()
void tst_QDBusPendingCall::watcher_waitForFinished_error()
{
QDBusPendingCall ac = sendError();
- QVERIFY(!ac.isFinished());
- QVERIFY(!ac.isError());
- QVERIFY(ac.reply().type() == QDBusMessage::InvalidMessage);
-
callCount = 0;
watchArgument = 0;
--
GitLab