From a5914a44a5a92364cad1e4a82a4b4f69f8e3909254fc1efa7024baead387af60 Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Thu, 9 Mar 2017 18:32:30 +0000 Subject: [PATCH] Accepting request 478010 from home:alarrosa:branches:KDE:Applications - Drop fix-itemretriever-in-case-of-concurrent-requests.patch, it's already merged upstream OBS-URL: https://build.opensuse.org/request/show/478010 OBS-URL: https://build.opensuse.org/package/show/KDE:Applications/akonadi-server?expand=0&rev=96 --- akonadi-server.changes | 6 + akonadi-server.spec | 3 - ...iever-in-case-of-concurrent-requests.patch | 296 ------------------ 3 files changed, 6 insertions(+), 299 deletions(-) delete mode 100644 fix-itemretriever-in-case-of-concurrent-requests.patch diff --git a/akonadi-server.changes b/akonadi-server.changes index a40da38..d8fae95 100644 --- a/akonadi-server.changes +++ b/akonadi-server.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Thu Mar 9 18:07:28 UTC 2017 - alarrosa@suse.com + +- Drop fix-itemretriever-in-case-of-concurrent-requests.patch, it's + already merged upstream + ------------------------------------------------------------------- Thu Mar 9 12:33:17 CET 2017 - lbeltrame@kde.org diff --git a/akonadi-server.spec b/akonadi-server.spec index 8986267..26296b4 100644 --- a/akonadi-server.spec +++ b/akonadi-server.spec @@ -31,8 +31,6 @@ Source: %{rname}-%{version}.tar.xz Source99: akonadi-server-rpmlintrc # PATCH-FIX-UPSTREAM handle-mysql-process-crashes-gracefully.patch Patch0: handle-mysql-process-crashes-gracefully.patch -# PATCH-FIX-UPSTREAM fix-itemretriever-in-case-of-concurrent-requests.patch - Fixes hangs when accessing folders -Patch1: fix-itemretriever-in-case-of-concurrent-requests.patch %if 0%{?suse_version} > 1325 BuildRequires: libboost_headers-devel %else @@ -172,7 +170,6 @@ service. %prep %setup -q -n %{rname}-%{version} %patch0 -p1 -%patch1 -p1 %build %cmake_kf5 -d build -- -DINSTALL_QSQLITE_IN_QT_PREFIX=TRUE -DQT_PLUGINS_DIR=%{_kf5_plugindir} diff --git a/fix-itemretriever-in-case-of-concurrent-requests.patch b/fix-itemretriever-in-case-of-concurrent-requests.patch deleted file mode 100644 index 01c720c..0000000 --- a/fix-itemretriever-in-case-of-concurrent-requests.patch +++ /dev/null @@ -1,296 +0,0 @@ -From d42f94701f38c26cf3439727b50ee73cf0d09bee Mon Sep 17 00:00:00 2001 -From: David Faure -Date: Wed, 15 Feb 2017 08:43:09 +0100 -Subject: Fix ItemRetriever in case of concurrent requests for the same item(s) - -Summary: -- ItemRetrievalManager must emit requestFinished() for those other requests, -otherwise the list of pending requests in ItemRetriever is never emptied - -- ItemRetriever must not assume that a signal being emitted by -ItemRetrievalManager is necessarily about the request it's waiting for, it -could be for another one. So it must first check in its list of pending -requests to determine whether it should react or not. - -With multithreaded unittest, checked for races with clang+tsan. -(There is one race, the connect to ItemRetrievalRequest vs the emit -in other threads, we should lock mLock before connect...) - -Test Plan: new unittest - -Reviewers: dvratil - -Reviewed By: dvratil - -Subscribers: #kde_pim - -Tags: #kde_pim - -Differential Revision: https://phabricator.kde.org/D4618 ---- - autotests/server/itemretrievertest.cpp | 174 ++++++++++++++++++++-------- - src/server/storage/itemretrievalmanager.cpp | 1 + - src/server/storage/itemretriever.cpp | 21 ++-- - 3 files changed, 135 insertions(+), 61 deletions(-) - -diff --git a/autotests/server/itemretrievertest.cpp b/autotests/server/itemretrievertest.cpp -index fb4468d..1206b35 100644 ---- a/autotests/server/itemretrievertest.cpp -+++ b/autotests/server/itemretrievertest.cpp -@@ -20,6 +20,7 @@ - #include - #include - #include -+#include - - #include "storage/itemretriever.h" - #include "storage/itemretrievaljob.h" -@@ -144,6 +145,52 @@ private: - QMultiHash mJobResults; - }; - -+using RequestedParts = QVector; -+ -+class ClientThread : public QThread -+{ -+public: -+ ClientThread(Entity::Id itemId, const RequestedParts &requestedParts) -+ : m_itemId(itemId), m_requestedParts(requestedParts) -+ {} -+ -+ void run() Q_DECL_OVERRIDE -+ { -+ // ItemRetriever should... -+ ItemRetriever retriever; -+ retriever.setItem(m_itemId); -+ retriever.setRetrieveParts(m_requestedParts); -+ QSignalSpy spy(&retriever, &ItemRetriever::itemsRetrieved); -+ -+ const bool success = retriever.exec(); -+ -+ QMutexLocker lock(&m_mutex); -+ m_results.success = success; -+ m_results.signalsCount = spy.count(); -+ if (m_results.signalsCount > 0) { -+ m_results.emittedItems = spy.at(0).at(0).value>(); -+ } -+ } -+ -+ struct Results -+ { -+ bool success; -+ int signalsCount; -+ QList emittedItems; -+ }; -+ Results results() const { -+ QMutexLocker lock(&m_mutex); -+ return m_results; -+ } -+ -+private: -+ const Entity::Id m_itemId; -+ const RequestedParts m_requestedParts; -+ -+ mutable QMutex m_mutex; // protects results below -+ Results m_results; -+}; -+ - class ItemRetrieverTest : public QObject - { - Q_OBJECT -@@ -151,7 +198,6 @@ class ItemRetrieverTest : public QObject - - using ExistingParts = QVector>; - using AvailableParts = QVector>; -- using RequestedParts = QVector; - - public: - ItemRetrieverTest() -@@ -252,63 +298,89 @@ private Q_SLOTS: - - - // Setup -- DbInitializer dbInitializer; -- FakeItemRetrievalJobFactory factory(dbInitializer); -- ItemRetrievalManager mgr(&factory); -- QTest::qWait(100); -- -- // Given a PimItem with existing parts -- Resource res = dbInitializer.createResource("testresource"); -- Collection col = dbInitializer.createCollection("col1"); -- PimItem item = dbInitializer.createItem("1", col); -- Q_FOREACH (const auto &existingPart, existingParts) { -- dbInitializer.createPart(item.id(), existingPart.first, existingPart.second); -- } -+ for (int step = 0; step < 2; ++step) { -+ DbInitializer dbInitializer; -+ FakeItemRetrievalJobFactory factory(dbInitializer); -+ ItemRetrievalManager mgr(&factory); -+ QTest::qWait(100); -+ -+ // Given a PimItem with existing parts -+ Resource res = dbInitializer.createResource("testresource"); -+ Collection col = dbInitializer.createCollection("col1"); -+ -+ // step 0: do it in the main thread, for easier debugging -+ PimItem item = dbInitializer.createItem("1", col); -+ Q_FOREACH (const auto &existingPart, existingParts) { -+ dbInitializer.createPart(item.id(), existingPart.first, existingPart.second); -+ } - -- Q_FOREACH (const auto &availablePart, availableParts) { -- factory.addJobResult(item.id(), availablePart.first, availablePart.second); -- } -+ Q_FOREACH (const auto &availablePart, availableParts) { -+ factory.addJobResult(item.id(), availablePart.first, availablePart.second); -+ } - -- // ItemRetriever should... -- ItemRetriever retriever; -- retriever.setItem(item.id()); -- retriever.setRetrieveParts(requestedParts); -- QSignalSpy spy(&retriever, &ItemRetriever::itemsRetrieved); -+ if (step == 0) { -+ ClientThread thread(item.id(), requestedParts); -+ thread.run(); -+ -+ const ClientThread::Results results = thread.results(); -+ // ItemRetriever should ... succeed -+ QVERIFY(results.success); -+ // Emit exactly one signal ... -+ QCOMPARE(results.signalsCount, expectedSignals); -+ // ... with that one item -+ if (expectedSignals > 0) { -+ QCOMPARE(results.emittedItems, QList{ item.id() }); -+ } - -- // Succeed -- QVERIFY(retriever.exec()); -- // Run exactly one retrieval job -- QCOMPARE(factory.jobsCount(), expectedRetrievalJobs); -- // Emit exactly one signal ... -- QCOMPARE(spy.count(), expectedSignals); -- // ... with that one item -- if (expectedSignals > 0) { -- QCOMPARE(spy.at(0).at(0).value>(), QList{ item.id() }); -- } -+ // Check that the factory had exactly one retrieval job -+ QCOMPARE(factory.jobsCount(), expectedRetrievalJobs); - -- // and the part exists in the DB -- const auto parts = item.parts(); -- QCOMPARE(parts.count(), expectedParts); -- Q_FOREACH (const Part &dbPart, item.parts()) { -- const QString fqname = dbPart.partType().ns() + QLatin1Char(':') + dbPart.partType().name(); -- if (!requestedParts.contains(fqname.toLatin1())) { -- continue; -+ } else { -+ QVector threads; -+ for (int i = 0; i < 20; ++i) { -+ threads.append(new ClientThread(item.id(), requestedParts)); -+ } -+ for (int i = 0; i < threads.size(); ++i) { -+ threads.at(i)->start(); -+ } -+ for (int i = 0; i < threads.size(); ++i) { -+ threads.at(i)->wait(); -+ } -+ for (int i = 0; i < threads.size(); ++i) { -+ const ClientThread::Results results = threads.at(i)->results(); -+ QVERIFY(results.success); -+ QCOMPARE(results.signalsCount, expectedSignals); -+ if (expectedSignals > 0) { -+ QCOMPARE(results.emittedItems, QList{ item.id() }); -+ } -+ } -+ qDeleteAll(threads); - } - -- auto it = std::find_if(availableParts.constBegin(), availableParts.constEnd(), -- [dbPart](const QPair &p) { -- return dbPart.partType().name().toLatin1() == p.first; -- }); -- if (it == availableParts.constEnd()) { -- it = std::find_if(existingParts.constBegin(), existingParts.constEnd(), -- [fqname](const QPair &p) { -- return fqname.toLatin1() == p.first; -- }); -- QVERIFY(it != existingParts.constEnd()); -- } -+ // Check that the parts now exist in the DB -+ const auto parts = item.parts(); -+ QCOMPARE(parts.count(), expectedParts); -+ Q_FOREACH (const Part &dbPart, item.parts()) { -+ const QString fqname = dbPart.partType().ns() + QLatin1Char(':') + dbPart.partType().name(); -+ if (!requestedParts.contains(fqname.toLatin1())) { -+ continue; -+ } -+ -+ auto it = std::find_if(availableParts.constBegin(), availableParts.constEnd(), -+ [dbPart](const QPair &p) { -+ return dbPart.partType().name().toLatin1() == p.first; -+ }); -+ if (it == availableParts.constEnd()) { -+ it = std::find_if(existingParts.constBegin(), existingParts.constEnd(), -+ [fqname](const QPair &p) { -+ return fqname.toLatin1() == p.first; -+ }); -+ QVERIFY(it != existingParts.constEnd()); -+ } - -- QCOMPARE(dbPart.data(), it->second); -- QCOMPARE(dbPart.datasize(), it->second.size()); -+ QCOMPARE(dbPart.data(), it->second); -+ QCOMPARE(dbPart.datasize(), it->second.size()); -+ } - } - } - }; -diff --git a/src/server/storage/itemretrievalmanager.cpp b/src/server/storage/itemretrievalmanager.cpp -index 65f5e78..382bb9e 100644 ---- a/src/server/storage/itemretrievalmanager.cpp -+++ b/src/server/storage/itemretrievalmanager.cpp -@@ -232,6 +232,7 @@ void ItemRetrievalManager::retrievalJobFinished(ItemRetrievalRequest *request, c - qCDebug(AKONADISERVER_LOG) << "someone else requested item" << request->ids << "as well, marking as processed"; - (*it)->errorMsg = errorMsg; - (*it)->processed = true; -+ Q_EMIT requestFinished(*it); - it = mPendingRequests[request->resourceId].erase(it); - } else { - ++it; -diff --git a/src/server/storage/itemretriever.cpp b/src/server/storage/itemretriever.cpp -index 0ae3e91..c57c860 100644 ---- a/src/server/storage/itemretriever.cpp -+++ b/src/server/storage/itemretriever.cpp -@@ -317,17 +317,18 @@ bool ItemRetriever::exec() - QEventLoop eventLoop; - connect(ItemRetrievalManager::instance(), &ItemRetrievalManager::requestFinished, - this, [&](ItemRetrievalRequest *finishedRequest) { -- if (!finishedRequest->errorMsg.isEmpty()) { -- mLastError = finishedRequest->errorMsg.toUtf8(); -- eventLoop.exit(1); -- } else { -- requests.removeOne(finishedRequest); -- Q_EMIT itemsRetrieved(finishedRequest->ids); -- if (requests.isEmpty()) { -- eventLoop.quit(); -- } -+ if (requests.removeOne(finishedRequest)) { -+ if (!finishedRequest->errorMsg.isEmpty()) { -+ mLastError = finishedRequest->errorMsg.toUtf8(); -+ eventLoop.exit(1); -+ } else { -+ Q_EMIT itemsRetrieved(finishedRequest->ids); -+ if (requests.isEmpty()) { -+ eventLoop.quit(); - } -- }, Qt::UniqueConnection); -+ } -+ } -+ }, Qt::UniqueConnection); - - auto it = requests.begin(); - while (it != requests.end()) { --- -cgit v0.11.2 -