From a5542751bc147c7f74131006339b55e6339b3e9f Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Dan=20Vr=C3=A1til?= <dvratil@redhat.com>

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 <trueg@kde.org>

+ *

+ * 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 <QCoreApplication>

+#include <QThread>

+#include <QThreadStorage>

+

+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<DBusConnectionPoolPrivate *> 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 <trueg@kde.org>

+ *

+ * 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 <QDBusConnection>

+

+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 <QtCore/QLocale>

 #include <QtCore/QStringList>

@@ -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 <trueg@kde.org>

- * Copyright (C) 2010 David Faure <faure@kde.org>

- *

- * 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 <QThreadStorage>

-

-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<DBusConnectionPoolPrivate *> 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 <trueg@kde.org>

- *

- * 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 <QtDBus/QDBusConnection>

-

-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 <dbusconnectionpool.h>

+#include "dbusconnectionpool.h"

 #include <QtDBus/QDBusInterface>

 #include <QtDBus/QDBusConnection>

@@ -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 <entities.h>

 #include <QSqlError>

@@ -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<QString, AgentSearchInstance* > 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 <akdbus.h>

 #include <akdebug.h>

@@ -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