From 4ecb10d3b1294d03578c28467c0f3759b3496e0b Mon Sep 17 00:00:00 2001
From: Christian Mollekopf <chrigi_1@fastmail.fm>
Date: Tue, 7 Mar 2017 12:04:47 +0100
Subject: Resolved potential deadlock

When trying to reply to a mail from kube we ran into a deadlock.
The initial result callback is called from the main thread, and that can
thus directly lead to destruction of the emitter.
---
 common/modelresult.cpp  | 18 +++++++++---------
 common/resultprovider.h | 10 +++++++++-
 tests/clientapitest.cpp | 20 ++++++++++++++------
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/common/modelresult.cpp b/common/modelresult.cpp
index f935419..904766d 100644
--- a/common/modelresult.cpp
+++ b/common/modelresult.cpp
@@ -296,15 +296,15 @@ void ModelResult<T, Ptr>::setEmitter(const typename Sink::ResultEmitter<Ptr>::Pt
     emitter->onInitialResultSetComplete([this, guard](const Ptr &parent, bool fetchedAll) {
         SinkTraceCtx(mLogCtx) << "Initial result set complete. Fetched all: " << fetchedAll;
         Q_ASSERT(guard);
-        threadBoundary.callInMainThread([=]() {
-            const qint64 parentId = parent ? qHash(*parent) : 0;
-            const auto parentIndex = createIndexFromId(parentId);
-            mEntityChildrenFetchComplete.insert(parentId);
-            if (fetchedAll) {
-                mEntityAllChildrenFetched.insert(parentId);
-            }
-            emit dataChanged(parentIndex, parentIndex, QVector<int>() << ChildrenFetchedRole);
-        });
+        Q_ASSERT(QThread::currentThread() == this->thread());
+
+        const qint64 parentId = parent ? qHash(*parent) : 0;
+        const auto parentIndex = createIndexFromId(parentId);
+        mEntityChildrenFetchComplete.insert(parentId);
+        if (fetchedAll) {
+            mEntityAllChildrenFetched.insert(parentId);
+        }
+        emit dataChanged(parentIndex, parentIndex, QVector<int>() << ChildrenFetchedRole);
     });
     mEmitter = emitter;
 }
diff --git a/common/resultprovider.h b/common/resultprovider.h
index a2ed0b5..d6feaf9 100644
--- a/common/resultprovider.h
+++ b/common/resultprovider.h
@@ -268,8 +268,9 @@ public:
 
     void initialResultSetComplete(const DomainType &parent, bool replayedAll)
     {
-        QMutexLocker locker{&mMutex};
+        //This callback is only ever called from the main thread, so we don't do any locking
         if (initialResultSetCompleteHandler && guardOk()) {
+            //This can directly lead to our destruction and thus waitForMethodExecutionEnd
             initialResultSetCompleteHandler(parent, replayedAll);
         }
     }
@@ -313,6 +314,13 @@ private:
     std::function<void(void)> clearHandler;
 
     std::function<void(const DomainType &parent)> mFetcher;
+    /*
+     * This mutex is here to protect the emitter from getting destroyed while the producer-thread (ResultProvider) is calling into it,
+     * and vice-verca, to protect the producer thread from calling into a destroyed emitter.
+     *
+     * This is necessary because Emitter and ResultProvider have lifetimes managed by two different threads.
+     * The emitter lives in the application thread, and the resultprovider in the query thread.
+     */
     QMutex mMutex;
     bool mDone = false;
 };
diff --git a/tests/clientapitest.cpp b/tests/clientapitest.cpp
index da52afb..3955ebd 100644
--- a/tests/clientapitest.cpp
+++ b/tests/clientapitest.cpp
@@ -11,6 +11,12 @@
 #include "test.h"
 #include "asyncutils.h"
 
+template <typename T>
+struct Result {
+    QSharedPointer<T> parent;
+    bool fetchedAll;
+};
+
 template <typename T>
 class TestDummyResourceFacade : public Sink::StoreFacade<T>
 {
@@ -68,7 +74,7 @@ public:
         auto emitter = resultProvider->emitter();
 
         resultProvider->setFetcher([query, resultProvider, this, ctx](const typename T::Ptr &parent) {
-            async::run<int>([=] {
+            async::run<Result<T>>([=] {
                 if (parent) {
                     SinkTraceCtx(ctx) << "Running the fetcher " << parent->identifier();
                 } else {
@@ -90,14 +96,16 @@ public:
                             SinkTraceCtx(ctx) << "Aborting early after " << count << "results.";
                             offset = i + 1;
                             bool fetchedAll = (i + 1 >= results.size());
-                            resultProvider->initialResultSetComplete(parent, fetchedAll);
-                            return 0;
+                            return Result<T>{parent, fetchedAll};
                         }
                     }
                 }
-                resultProvider->initialResultSetComplete(parent, true);
-                return 0;
-            }, runAsync).exec();
+                return Result<T>{parent, true};
+            }, runAsync)
+            .then([=] (const Result<T> &r) {
+                resultProvider->initialResultSetComplete(r.parent, r.fetchedAll);
+            })
+            .exec();
         });
         auto job = KAsync::start([query, resultProvider]() {});
         mResultProvider = resultProvider.data();
-- 
cgit v1.2.3