From a5542751bc147c7f74131006339b55e6339b3e9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20Vr=C3=A1til?= Date: Thu, 17 Apr 2014 15:11:57 +0200 Subject: [PATCH 1/7] Use per-thread QDBusConnections This moves DBusConnectionPool from Nepomuk search code to Akonadi and makes use of it in search infrastructure and couple other classes that interact with DBus from non-main thread. QDBusConnection is not thread-safe in Qt 4, so we need to workaround it by having a connection for each thread. Qt 5 should be OK, so we can remove this in Frameworks. This should fix random crashes I've been seeing when SearchTaskManager::addTask() was called from multiple threads simultaneously. --- server/CMakeLists.txt | 2 +- server/src/dbusconnectionpool.cpp | 62 +++++++++++++++++++++++++++++ server/src/dbusconnectionpool.h | 41 +++++++++++++++++++ server/src/handler/fetchhelper.cpp | 9 ++--- server/src/nepomuk/dbusconnectionpool.cpp | 59 --------------------------- server/src/nepomuk/dbusconnectionpool.h | 38 ------------------ server/src/nepomuk/queryserviceclient.cpp | 4 +- server/src/search/agentsearchinstance.cpp | 5 ++- server/src/search/searchmanager.cpp | 3 +- server/src/search/searchtaskmanager.cpp | 11 ++--- server/src/search/searchtaskmanager.h | 2 - server/src/storage/itemretrievalmanager.cpp | 6 +-- server/src/storagejanitor.cpp | 3 +- 13 files changed, 124 insertions(+), 121 deletions(-) create mode 100644 server/src/dbusconnectionpool.cpp create mode 100644 server/src/dbusconnectionpool.h delete mode 100644 server/src/nepomuk/dbusconnectionpool.cpp delete mode 100644 server/src/nepomuk/dbusconnectionpool.h diff --git a/server/CMakeLists.txt b/server/CMakeLists.txt index 571bead..1709348 100644 --- a/server/CMakeLists.txt +++ b/server/CMakeLists.txt @@ -106,6 +106,7 @@ set(libakonadiprivate_SRCS src/collectionscheduler.cpp src/clientcapabilities.cpp src/clientcapabilityaggregator.cpp + src/dbusconnectionpool.cpp src/handler.cpp src/handlerhelper.cpp src/intervalcheck.cpp @@ -206,7 +207,6 @@ if (Soprano_FOUND) src/search/nepomuksearchengine.cpp src/nepomuk/dbusoperators.cpp src/nepomuk/queryserviceclient.cpp - src/nepomuk/dbusconnectionpool.cpp src/nepomuk/result.cpp ) diff --git a/server/src/dbusconnectionpool.cpp b/server/src/dbusconnectionpool.cpp new file mode 100644 index 0000000..9aede4f --- /dev/null +++ b/server/src/dbusconnectionpool.cpp @@ -0,0 +1,62 @@ +/* + * Copyright (C) 2010 Sebastian Trueg + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public License + * along with this library; see the file COPYING.LIB. If not, write to + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ + +#include "dbusconnectionpool.h" +#include +#include +#include + +namespace { +QAtomicInt s_connectionCounter; + +class DBusConnectionPoolPrivate +{ +public: + DBusConnectionPoolPrivate() + : m_connection( QDBusConnection::connectToBus( + QDBusConnection::SessionBus, + QString::fromLatin1("AkonadiServer-%1").arg(newNumber()) ) ) + { + } + ~DBusConnectionPoolPrivate() { + QDBusConnection::disconnectFromBus( m_connection.name() ); + } + + QDBusConnection connection() const { return m_connection; } + +private: + static int newNumber() { + return s_connectionCounter.fetchAndAddAcquire( 1 ); + } + QDBusConnection m_connection; +}; +} + +QThreadStorage s_perThreadConnection; + +QDBusConnection Akonadi::Server::DBusConnectionPool::threadConnection() +{ + if ( !QCoreApplication::instance() || QCoreApplication::instance()->thread() == QThread::currentThread() ) { + return QDBusConnection::sessionBus(); // main thread, use the default session bus + } + if ( !s_perThreadConnection.hasLocalData() ) { + s_perThreadConnection.setLocalData( new DBusConnectionPoolPrivate ); + } + return s_perThreadConnection.localData()->connection(); +} diff --git a/server/src/dbusconnectionpool.h b/server/src/dbusconnectionpool.h new file mode 100644 index 0000000..4f8a93e --- /dev/null +++ b/server/src/dbusconnectionpool.h @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2010 Sebastian Trueg + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public License + * along with this library; see the file COPYING.LIB. If not, write to + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ + +#ifndef DBUSCONNECTIONPOOL_H +#define DBUSCONNECTIONPOOL_H + +#include + +namespace Akonadi { +namespace Server { +namespace DBusConnectionPool { + +/** + * Returns a new QDBusConnection for each thread, because QDBusConnection is + * not thread-safe in Qt 4. + * + * FIXME: Remove in KF5 + */ +QDBusConnection threadConnection(); + +} +} +} + +#endif diff --git a/server/src/handler/fetchhelper.cpp b/server/src/handler/fetchhelper.cpp index 6284cc9..a6888a3 100644 --- a/server/src/handler/fetchhelper.cpp +++ b/server/src/handler/fetchhelper.cpp @@ -38,6 +38,7 @@ #include "utils.h" #include "intervalcheck.h" #include "agentmanagerinterface.h" +#include "dbusconnectionpool.h" #include #include @@ -252,16 +253,12 @@ bool FetchHelper::isScopeLocal( const Scope &scope ) query.next(); const QString resourceName = query.value( 0 ).toString(); - // Workaround for QDBusConnectionPrivate not being thread-safe in Qt 4, fixed in Qt 5.2 - // TODO: Remove in KF5 - const QDBusConnection connection = QDBusConnection::connectToBus( QDBusConnection::SessionBus, - QString::fromLatin1( mConnection->sessionId() ) ); + org::freedesktop::Akonadi::AgentManager manager( AkDBus::serviceName( AkDBus::Control ), QLatin1String( "/AgentManager" ), - connection ); + DBusConnectionPool::threadConnection() ); const QString typeIdentifier = manager.agentInstanceType( resourceName ); const QVariantMap properties = manager.agentCustomProperties( typeIdentifier ); - QDBusConnection::disconnectFromBus( QString::fromLatin1( mConnection->sessionId() ) ); return properties.value( QLatin1String( "HasLocalStorage" ), false ).toBool(); } diff --git a/server/src/nepomuk/dbusconnectionpool.cpp b/server/src/nepomuk/dbusconnectionpool.cpp deleted file mode 100644 index 598d16f..0000000 --- a/server/src/nepomuk/dbusconnectionpool.cpp +++ /dev/null @@ -1,59 +0,0 @@ -/* - * This file is part of the Nepomuk KDE project. - * Copyright (C) 2010 Sebastian Trueg - * Copyright (C) 2010 David Faure - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Library General Public - * License as published by the Free Software Foundation; either - * version 2 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Library General Public License for more details. - * - * You should have received a copy of the GNU Library General Public License - * along with this library; see the file COPYING.LIB. If not, write to - * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, - * Boston, MA 02110-1301, USA. - */ - -#include "dbusconnectionpool.h" -#include - -namespace { -QAtomicInt s_connectionCounter; - -class DBusConnectionPoolPrivate -{ -public: - DBusConnectionPoolPrivate() - : m_connection( QDBusConnection::connectToBus( - QDBusConnection::SessionBus, - QString::fromLatin1( "NepomukQueryServiceConnection%1" ).arg( newNumber() ) ) ) - { - } - ~DBusConnectionPoolPrivate() { - QDBusConnection::disconnectFromBus( m_connection.name() ); - } - - QDBusConnection connection() const { return m_connection; } - -private: - static int newNumber() { - return s_connectionCounter.fetchAndAddAcquire( 1 ); - } - QDBusConnection m_connection; -}; -} - -QThreadStorage s_perThreadConnection; - -QDBusConnection DBusConnectionPool::threadConnection() -{ - if ( !s_perThreadConnection.hasLocalData() ) { - s_perThreadConnection.setLocalData( new DBusConnectionPoolPrivate ); - } - return s_perThreadConnection.localData()->connection(); -} diff --git a/server/src/nepomuk/dbusconnectionpool.h b/server/src/nepomuk/dbusconnectionpool.h deleted file mode 100644 index c5ac746..0000000 --- a/server/src/nepomuk/dbusconnectionpool.h +++ /dev/null @@ -1,38 +0,0 @@ -/* - * This file is part of the Nepomuk KDE project. - * Copyright (C) 2010 Sebastian Trueg - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Library General Public - * License as published by the Free Software Foundation; either - * version 2 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Library General Public License for more details. - * - * You should have received a copy of the GNU Library General Public License - * along with this library; see the file COPYING.LIB. If not, write to - * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, - * Boston, MA 02110-1301, USA. - */ - -#ifndef _DBUS_CONNECTION_POOL_H_ -#define _DBUS_CONNECTION_POOL_H_ - -#include - -namespace DBusConnectionPool -{ -/** - * The DBusConnectionPool works around the problem - * of QDBusConnection not being thread-safe. As soon as that - * has been fixed (either directly in libdbus or with a work- - * around in Qt) this method can be dropped in favor of - * QDBusConnection::sessionBus(). - */ -QDBusConnection threadConnection(); -} - -#endif diff --git a/server/src/nepomuk/queryserviceclient.cpp b/server/src/nepomuk/queryserviceclient.cpp index 089a80b..b27a751 100644 --- a/server/src/nepomuk/queryserviceclient.cpp +++ b/server/src/nepomuk/queryserviceclient.cpp @@ -24,7 +24,7 @@ #include "result.h" #include "queryserviceinterface.h" #include "queryinterface.h" -#include +#include "dbusconnectionpool.h" #include #include @@ -40,7 +40,7 @@ public: Private() : queryServiceInterface( 0 ), queryInterface( 0 ), - dbusConnection( DBusConnectionPool::threadConnection() ), + dbusConnection( Akonadi::Server::DBusConnectionPool::threadConnection() ), m_queryActive( false ), loop( 0 ) { } diff --git a/server/src/search/agentsearchinstance.cpp b/server/src/search/agentsearchinstance.cpp index 54ffab0..ca6ef87 100644 --- a/server/src/search/agentsearchinstance.cpp +++ b/server/src/search/agentsearchinstance.cpp @@ -21,6 +21,7 @@ #include "agentsearchinterface.h" #include "searchtaskmanager.h" #include "akdbus.h" +#include "dbusconnectionpool.h" using namespace Akonadi::Server; @@ -43,7 +44,7 @@ bool AgentSearchInstance::init() mInterface = new OrgFreedesktopAkonadiAgentSearchInterface( AkDBus::agentServiceName( mId, AkDBus::Agent ), QLatin1String( "/Search" ), - QDBusConnection::sessionBus() ); + DBusConnectionPool::threadConnection() ); if ( !mInterface || !mInterface->isValid() ) { delete mInterface; @@ -52,7 +53,7 @@ bool AgentSearchInstance::init() } mServiceWatcher = new QDBusServiceWatcher( AkDBus::agentServiceName( mId, AkDBus::Agent ), - QDBusConnection::sessionBus(), + DBusConnectionPool::threadConnection(), QDBusServiceWatcher::WatchForOwnerChange, this ); connect( mServiceWatcher, SIGNAL(serviceOwnerChanged(QString,QString,QString)), diff --git a/server/src/search/searchmanager.cpp b/server/src/search/searchmanager.cpp index 139d058..1eafb27 100644 --- a/server/src/search/searchmanager.cpp +++ b/server/src/search/searchmanager.cpp @@ -26,6 +26,7 @@ #include "agentsearchengine.h" #include "nepomuksearchengine.h" #include "notificationmanager.h" +#include "dbusconnectionpool.h" #include "searchrequest.h" #include "searchtaskmanager.h" #include "storage/datastore.h" @@ -98,7 +99,7 @@ SearchManager::SearchManager( const QStringList &searchEngines, QObject *parent new SearchManagerAdaptor( this ); - QDBusConnection::sessionBus().registerObject( + DBusConnectionPool::threadConnection().registerObject( QLatin1String( "/SearchManager" ), this, QDBusConnection::ExportAdaptors ); diff --git a/server/src/search/searchtaskmanager.cpp b/server/src/search/searchtaskmanager.cpp index 51bb516..e7980a4 100644 --- a/server/src/search/searchtaskmanager.cpp +++ b/server/src/search/searchtaskmanager.cpp @@ -23,6 +23,7 @@ #include "akdbus.h" #include "connection.h" #include "storage/selectquerybuilder.h" +#include "dbusconnectionpool.h" #include #include @@ -36,9 +37,6 @@ SearchTaskManager *SearchTaskManager::sInstance = 0; SearchTaskManager::SearchTaskManager() : QObject() , mShouldStop( false ) - , mAgentManager( AkDBus::serviceName( AkDBus::Control ), QLatin1String( "/AgentManager" ), - QDBusConnection::sessionBus() ) - { sInstance = this; @@ -125,13 +123,16 @@ void SearchTaskManager::addTask( SearchTask *task ) } mInstancesLock.lock(); + + org::freedesktop::Akonadi::AgentManager agentManager( AkDBus::serviceName( AkDBus::Control ), QLatin1String( "/AgentManager" ), + DBusConnectionPool::threadConnection() ); do { const QString resourceId = query.value( 1 ).toString(); if ( !mInstances.contains( resourceId ) ) { akDebug() << "Resource" << resourceId << "does not implement Search interface, skipping"; - } else if ( !mAgentManager.agentInstanceOnline( resourceId ) ) { + } else if ( !agentManager.agentInstanceOnline( resourceId ) ) { akDebug() << "Agent" << resourceId << "is offline, skipping"; - } else if ( mAgentManager.agentInstanceStatus( resourceId ) > 2 ) { // 2 == Broken, 3 == Not Configured + } else if ( agentManager.agentInstanceStatus( resourceId ) > 2 ) { // 2 == Broken, 3 == Not Configured akDebug() << "Agent" << resourceId << "is broken or not configured"; } else { const qint64 collectionId = query.value( 0 ).toLongLong(); diff --git a/server/src/search/searchtaskmanager.h b/server/src/search/searchtaskmanager.h index 06e1b52..9b7972b 100644 --- a/server/src/search/searchtaskmanager.h +++ b/server/src/search/searchtaskmanager.h @@ -97,8 +97,6 @@ class SearchTaskManager : public QObject TasksMap::Iterator cancelRunningTask( TasksMap::Iterator &iter ); bool allResourceTasksCompleted( SearchTask* ) const; - org::freedesktop::Akonadi::AgentManager mAgentManager; - QMap mInstances; QMutex mInstancesLock; diff --git a/server/src/storage/itemretrievalmanager.cpp b/server/src/storage/itemretrievalmanager.cpp index d49ba66..ce88410 100644 --- a/server/src/storage/itemretrievalmanager.cpp +++ b/server/src/storage/itemretrievalmanager.cpp @@ -20,6 +20,7 @@ #include "itemretrievalmanager.h" #include "itemretrievalrequest.h" #include "itemretrievaljob.h" +#include "dbusconnectionpool.h" #include "resourceinterface.h" @@ -40,10 +41,7 @@ ItemRetrievalManager *ItemRetrievalManager::sInstance = 0; ItemRetrievalManager::ItemRetrievalManager( QObject *parent ) : QObject( parent ), - mDBusConnection( - QDBusConnection::connectToBus( - QDBusConnection::SessionBus, - QString::fromLatin1( "AkonadiServerItemRetrievalManager" ) ) ) + mDBusConnection( DBusConnectionPool::threadConnection() ) { // make sure we are created from the retrieval thread and only once Q_ASSERT( QThread::currentThread() != QCoreApplication::instance()->thread() ); diff --git a/server/src/storagejanitor.cpp b/server/src/storagejanitor.cpp index ebe7a24..af1a407 100644 --- a/server/src/storagejanitor.cpp +++ b/server/src/storagejanitor.cpp @@ -26,6 +26,7 @@ #include "storage/parthelper.h" #include "resourcemanager.h" #include "entities.h" +#include "dbusconnectionpool.h" #include #include @@ -63,7 +64,7 @@ void StorageJanitorThread::run() StorageJanitor::StorageJanitor( QObject *parent ) : QObject( parent ) - , m_connection( QDBusConnection::connectToBus( QDBusConnection::SessionBus, QLatin1String( staticMetaObject.className() ) ) ) + , m_connection( DBusConnectionPool::threadConnection() ) , m_lostFoundCollectionId( -1 ) { DataStore::self(); -- 1.9.3