TaskTree: Make storages thread-safe

Make it possible to safely run concurrently 2 task trees
in 2 separate threads containing the common recipe with
common storages.

This addresses the 24th point in the master task below.

Task-number: QTCREATORBUG-28741
Change-Id: I3a413277e1f0640c38d6b85236e9aca09552e38f
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: hjk <hjk@qt.io>

src/libs/solutions/tasking/tasktree.cpp

@@ -843,67 +843,101 @@ bool TreeStorageBase::isValid() const
 }
 
 TreeStorageBase::TreeStorageBase(StorageConstructor ctor, StorageDestructor dtor)
-    : m_storageData(new StorageData{ctor, dtor}) { }
+    : m_storageData(new StorageData{ctor, dtor})
+{}
 
-TreeStorageBase::StorageData::~StorageData()
+TreeStorageBase::ThreadData::ThreadData(TreeStorageBase::StorageData *storageData)
+    : m_storageData(storageData)
+{
+    QTC_CHECK(m_storageData->m_constructor);
+    QTC_CHECK(m_storageData->m_destructor);
+}
+
+TreeStorageBase::ThreadData::~ThreadData()
 {
     QTC_CHECK(m_storageHash.isEmpty());
+    // TODO: Issue a warning about the leak instead?
     for (void *ptr : std::as_const(m_storageHash))
-        m_destructor(ptr);
+        m_storageData->m_destructor(ptr);
 }
 
-void *TreeStorageBase::activeStorageVoid() const
+int TreeStorageBase::ThreadData::createStorage()
 {
-    QTC_ASSERT(m_storageData->m_activeStorage, qWarning(
+    QTC_ASSERT(m_activeStorage == 0, return 0); // TODO: should be allowed?
-        "The referenced storage is not reachable in the running tree. "
+    const int newId = m_storageData->m_storageInstanceCounter.fetch_add(1) + 1;
-        "A nullptr will be returned which might lead to a crash in the calling code. "
+    m_storageHash.insert(newId, m_storageData->m_constructor());
-        "It is possible that no storage was added to the tree, "
+    return newId;
-        "or the storage is not reachable from where it is referenced.");
-        return nullptr);
-    const auto it = m_storageData->m_storageHash.constFind(m_storageData->m_activeStorage);
-    QTC_ASSERT(it != m_storageData->m_storageHash.constEnd(), return nullptr);
-    return it.value();
 }
 
-int TreeStorageBase::activeStorageId() const
+bool TreeStorageBase::ThreadData::deleteStorage(int id)
 {
-    return m_storageData->m_activeStorage;
+    QTC_ASSERT(m_activeStorage == 0, return false); // TODO: should be allowed?
+    const auto it = m_storageHash.constFind(id);
+    QTC_ASSERT(it != m_storageHash.constEnd(), return false);
+    m_storageData->m_destructor(it.value());
+    m_storageHash.erase(it);
+    return m_storageHash.empty();
 }
 
 int TreeStorageBase::createStorage() const
 {
-    QTC_ASSERT(m_storageData->m_constructor, return 0); // TODO: add isValid()?
+    return threadData().createStorage();
-    QTC_ASSERT(m_storageData->m_destructor, return 0);
-    QTC_ASSERT(m_storageData->m_activeStorage == 0, return 0); // TODO: should be allowed?
-    const int newId = ++m_storageData->m_storageCounter;
-    m_storageData->m_storageHash.insert(newId, m_storageData->m_constructor());
-    return newId;
 }
 
 void TreeStorageBase::deleteStorage(int id) const
 {
-    QTC_ASSERT(m_storageData->m_constructor, return); // TODO: add isValid()?
+    if (!threadData().deleteStorage(id))
-    QTC_ASSERT(m_storageData->m_destructor, return);
+        return;
-    QTC_ASSERT(m_storageData->m_activeStorage == 0, return); // TODO: should be allowed?
+
-    const auto it = m_storageData->m_storageHash.constFind(id);
+    QMutexLocker lock(&m_storageData->m_threadDataMutex);
-    QTC_ASSERT(it != m_storageData->m_storageHash.constEnd(), return);
+    m_storageData->m_threadDataMap.erase(
-    m_storageData->m_destructor(it.value());
+        m_storageData->m_threadDataMap.find(QThread::currentThread()));
-    m_storageData->m_storageHash.erase(it);
 }
 
+
 // passing 0 deactivates currently active storage
-void TreeStorageBase::activateStorage(int id) const
+void TreeStorageBase::ThreadData::activateStorage(int id)
 {
     if (id == 0) {
-        QTC_ASSERT(m_storageData->m_activeStorage, return);
+        QTC_ASSERT(m_activeStorage, return);
-        m_storageData->m_activeStorage = 0;
+        m_activeStorage = 0;
         return;
     }
-    QTC_ASSERT(m_storageData->m_activeStorage == 0, return);
+    QTC_ASSERT(m_activeStorage == 0, return);
     // TODO: Unneeded check? OTOH, it's quite important...
-    const auto it = m_storageData->m_storageHash.find(id);
+    const auto it = m_storageHash.find(id);
-    QTC_ASSERT(it != m_storageData->m_storageHash.end(), return);
+    QTC_ASSERT(it != m_storageHash.end(), return);
-    m_storageData->m_activeStorage = id;
+    m_activeStorage = id;
+}
+
+void *TreeStorageBase::ThreadData::activeStorageVoid() const
+{
+    QTC_ASSERT(m_activeStorage, qWarning(
+        "The referenced storage is not reachable in the running tree. "
+        "A nullptr will be returned which might lead to a crash in the calling code. "
+        "It is possible that no storage was added to the tree, "
+        "or the storage is not reachable from where it is referenced."); return nullptr);
+    const auto it = m_storageHash.constFind(m_activeStorage);
+    QTC_ASSERT(it != m_storageHash.constEnd(), return nullptr);
+    return it.value();
+}
+
+int TreeStorageBase::ThreadData::activeStorageId() const
+{
+    return m_activeStorage;
+}
+
+TreeStorageBase::StorageData::~StorageData()
+{
+    QMutexLocker lock(&m_threadDataMutex);
+    QTC_CHECK(m_threadDataMap.empty());
+}
+
+TreeStorageBase::ThreadData &TreeStorageBase::threadData() const
+{
+    QMutexLocker lock(&m_storageData->m_threadDataMutex);
+    return m_storageData->m_threadDataMap.emplace(QThread::currentThread(),
+                                                  m_storageData.get()).first->second;
 }
 
 void GroupItem::addChildren(const QList<GroupItem> &children)
@@ -1017,10 +1051,13 @@ public:
             return;
         GuardLocker locker(m_guard);
         const StorageHandler storageHandler = *it;
-        storage.activateStorage(storageId);
+        // TODO: we don't necessarily need to activate the storage here, it's enough to
+        // get a pointer to the relevant storage instance.
+        auto &threadData = storage.threadData();
+        threadData.activateStorage(storageId);
         if (storageHandler.*ptr)
-            (storageHandler.*ptr)(storage.activeStorageVoid());
+            (storageHandler.*ptr)(threadData.activeStorageVoid());
-        storage.activateStorage(0);
+        threadData.activateStorage(0);
     }
 
     TaskTree *q = nullptr;
@@ -1170,7 +1207,7 @@ public:
     ExecutionContextActivator(TaskContainer *container) { activateContext(container); }
     ~ExecutionContextActivator() {
         for (int i = m_activeStorages.size() - 1; i >= 0; --i) // iterate in reverse order
-            m_activeStorages[i].activateStorage(0);
+            m_activeStorages[i].threadData().activateStorage(0);
     }
 
 private:
@@ -1180,10 +1217,11 @@ private:
         const TaskContainer::ConstData &constData = container->m_constData;
         for (int i = 0; i < constData.m_storageList.size(); ++i) {
             const TreeStorageBase &storage = constData.m_storageList[i];
-            if (storage.activeStorageId())
+            auto &threadData = storage.threadData();
+            if (threadData.activeStorageId())
                 continue; // Storage shadowing: The storage is already active, skipping it...
             m_activeStorages.append(storage);
-            storage.activateStorage(container->m_runtimeData->m_storageIdList.value(i));
+            threadData.activateStorage(container->m_runtimeData->m_storageIdList.value(i));
         }
         // Go to the parent after activating this storages so that storage shadowing works
         // in the direction from child to parent root.

src/libs/solutions/tasking/tasktree.h

@@ -6,9 +6,11 @@
 #include "tasking_global.h"
 
 #include <QHash>
+#include <QMutex>
 #include <QObject>
 #include <QSharedPointer>
 
+#include <atomic>
 #include <memory>
 
 QT_BEGIN_NAMESPACE
@@ -51,12 +53,33 @@ private:
     using StorageDestructor = std::function<void(void *)>;
 
     TreeStorageBase(StorageConstructor ctor, StorageDestructor dtor);
-    void *activeStorageVoid() const;
-    int activeStorageId() const;
 
+    struct StorageData;
+
+    struct TASKING_EXPORT ThreadData
+    {
+        Q_DISABLE_COPY_MOVE(ThreadData)
+
+        ThreadData(StorageData *storageData);
+        ~ThreadData();
+
+        int createStorage();
+        bool deleteStorage(int id); // Returns true if it was the last storage.
+
+        void activateStorage(int id);
+
+        void *activeStorageVoid() const;
+        int activeStorageId() const;
+
+    private:
+        StorageData *m_storageData = nullptr;
+        QHash<int, void *> m_storageHash;
+        int m_activeStorage = 0; // 0 means no active storage.
+    };
+
+    ThreadData &threadData() const;
     int createStorage() const;
     void deleteStorage(int id) const;
-    void activateStorage(int id) const;
 
     friend bool operator==(const TreeStorageBase &first, const TreeStorageBase &second)
     { return first.m_storageData == second.m_storageData; }
@@ -67,13 +90,16 @@ private:
     friend size_t qHash(const TreeStorageBase &storage, uint seed = 0)
     { return size_t(storage.m_storageData.get()) ^ seed; }
 
-    struct StorageData {
+    struct StorageData
+    {
         ~StorageData();
-        StorageConstructor m_constructor = {};
+        const StorageConstructor m_constructor = {};
-        StorageDestructor m_destructor = {};
+        const StorageDestructor m_destructor = {};
-        QHash<int, void *> m_storageHash = {};
+        QMutex m_threadDataMutex = {};
-        int m_activeStorage = 0; // 0 means no active storage
+        // Use std::map on purpose, so that it doesn't invalidate references on modifications.
-        int m_storageCounter = 0;
+        // Don't optimize it by using std::unordered_map.
+        std::map<QThread *, ThreadData> m_threadDataMap = {};
+        std::atomic_int m_storageInstanceCounter = 0; // Bumped on each creation.
     };
     QSharedPointer<StorageData> m_storageData;
 
@@ -91,7 +117,7 @@ public:
     StorageStruct &operator*() const noexcept { return *activeStorage(); }
     StorageStruct *operator->() const noexcept { return activeStorage(); }
     StorageStruct *activeStorage() const {
-        return static_cast<StorageStruct *>(activeStorageVoid());
+        return static_cast<StorageStruct *>(threadData().activeStorageVoid()); // HERE
     }
 
 private: