fix: race condition on ResourceFolderModel tests

This (hopefully) fixes the race contiditions that sometimes got
triggered in tests.

Signed-off-by: flow <flowlnlnln@gmail.com>

launcher/minecraft/mod/ModFolderModel.cpp

@@ -151,7 +151,7 @@ int ModFolderModel::columnCount(const QModelIndex &parent) const
 Task* ModFolderModel::createUpdateTask()
 {
     auto index_dir = indexDir();
-    auto task = new ModFolderLoadTask(dir(), index_dir, m_is_indexed, m_first_folder_load, this);
+    auto task = new ModFolderLoadTask(dir(), index_dir, m_is_indexed, m_first_folder_load);
     m_first_folder_load = false;
     return task;
 }
@@ -259,15 +259,6 @@ void ModFolderModel::onUpdateSucceeded()
 #endif
 
     applyUpdates(current_set, new_set, new_mods);
-    
-    m_current_update_task.reset();
-
-    if (m_scheduled_update) {
-        m_scheduled_update = false;
-        update();
-    } else {
-        emit updateFinished();
-    }
 }
 
 void ModFolderModel::onParseSucceeded(int ticket, QString mod_id)

launcher/minecraft/mod/ResourceFolderModel.cpp

@@ -1,5 +1,6 @@
 #include "ResourceFolderModel.h"
 
+#include <QCoreApplication>
 #include <QDebug>
 #include <QMimeData>
 #include <QThreadPool>
@@ -19,6 +20,12 @@ ResourceFolderModel::ResourceFolderModel(QDir dir, QObject* parent) : QAbstractL
     connect(&m_watcher, &QFileSystemWatcher::directoryChanged, this, &ResourceFolderModel::directoryChanged);
 }
 
+ResourceFolderModel::~ResourceFolderModel()
+{
+    while (!QThreadPool::globalInstance()->waitForDone(100))
+        QCoreApplication::processEvents();
+}
+
 bool ResourceFolderModel::startWatching(const QStringList paths)
 {
     if (m_is_watching)
@@ -229,9 +236,17 @@ bool ResourceFolderModel::update()
     connect(m_current_update_task.get(), &Task::succeeded, this, &ResourceFolderModel::onUpdateSucceeded,
             Qt::ConnectionType::QueuedConnection);
     connect(m_current_update_task.get(), &Task::failed, this, &ResourceFolderModel::onUpdateFailed, Qt::ConnectionType::QueuedConnection);
+    connect(m_current_update_task.get(), &Task::finished, this, [=] {
+        m_current_update_task.reset();
+        if (m_scheduled_update) {
+            m_scheduled_update = false;
+            update();
+        } else {
+            emit updateFinished();
+        }
+    }, Qt::ConnectionType::QueuedConnection);
 
-    auto* thread_pool = QThreadPool::globalInstance();
+    QThreadPool::globalInstance()->start(m_current_update_task.get());
-    thread_pool->start(m_current_update_task.get());
 
     return true;
 }
@@ -246,10 +261,7 @@ void ResourceFolderModel::resolveResource(Resource::Ptr res)
     if (!task)
         return;
 
-    m_ticket_mutex.lock();
+    int ticket = m_next_resolution_ticket.fetch_add(1);
-    int ticket = m_next_resolution_ticket;
-    m_next_resolution_ticket += 1;
-    m_ticket_mutex.unlock();
 
     res->setResolving(true, ticket);
     m_active_parse_tasks.insert(ticket, task);
@@ -261,8 +273,7 @@ void ResourceFolderModel::resolveResource(Resource::Ptr res)
     connect(
         task, &Task::finished, this, [=] { m_active_parse_tasks.remove(ticket); }, Qt::ConnectionType::QueuedConnection);
 
-    auto* thread_pool = QThreadPool::globalInstance();
+    QThreadPool::globalInstance()->start(task);
-    thread_pool->start(task);
 }
 
 void ResourceFolderModel::onUpdateSucceeded()
@@ -283,15 +294,6 @@ void ResourceFolderModel::onUpdateSucceeded()
 #endif
 
     applyUpdates(current_set, new_set, new_resources);
-
-    m_current_update_task.reset();
-
-    if (m_scheduled_update) {
-        m_scheduled_update = false;
-        update();
-    } else {
-        emit updateFinished();
-    }
 }
 
 void ResourceFolderModel::onParseSucceeded(int ticket, QString resource_id)

launcher/minecraft/mod/ResourceFolderModel.h

@@ -24,6 +24,7 @@ class ResourceFolderModel : public QAbstractListModel {
     Q_OBJECT
    public:
     ResourceFolderModel(QDir, QObject* parent = nullptr);
+    ~ResourceFolderModel() override;
 
     /** Starts watching the paths for changes.
      *
@@ -197,8 +198,7 @@ class ResourceFolderModel : public QAbstractListModel {
     QMap<QString, int> m_resources_index;
 
     QMap<int, Task::Ptr> m_active_parse_tasks;
-    int m_next_resolution_ticket = 0;
+    std::atomic<int> m_next_resolution_ticket = 0;
-    QMutex m_ticket_mutex;
 };
 
 /* A macro to define useful functions to handle Resource* -> T* more easily on derived classes */

launcher/minecraft/mod/ResourceFolderModel_test.cpp

@@ -58,7 +58,7 @@
     QVERIFY2(expire_timer.isActive(), "Timer has expired. The update never finished."); \
     expire_timer.stop();                                                                \
                                                                                         \
-    disconnect(&model, nullptr, nullptr, nullptr);
+    disconnect(&model, nullptr, &loop, nullptr);
 
 class ResourceFolderModelTest : public QObject
 {
@@ -150,11 +150,6 @@ slots:
         QCOMPARE(model.size(), 4);
 
         model.stopWatching();
-
-        while (model.hasPendingParseTasks()) {
-            QTest::qSleep(20);
-            QCoreApplication::processEvents();
-        }
     }
 
     void test_removeResource()
@@ -207,11 +202,6 @@ slots:
         qDebug() << "Removed second mod.";
 
         model.stopWatching();
-
-        while (model.hasPendingParseTasks()) {
-            QTest::qSleep(20);
-            QCoreApplication::processEvents();
-        }
     }
 
     void test_enable_disable()
@@ -263,11 +253,6 @@ slots:
         QVERIFY(!res_2.enable(initial_enabled_res_2 ? EnableAction::ENABLE : EnableAction::DISABLE));
         QVERIFY(res_2.enabled() == initial_enabled_res_2);
         QVERIFY(res_2.internal_id() == id_2);
-
-        while (model.hasPendingParseTasks()) {
-            QTest::qSleep(20);
-            QCoreApplication::processEvents();
-        }
     }
 };
 

launcher/minecraft/mod/tasks/BasicFolderLoadTask.h

@@ -36,7 +36,7 @@ class BasicFolderLoadTask : public Task {
     [[nodiscard]] bool canAbort() const override { return true; }
     bool abort() override
     {
-        m_aborted = true;
+        m_aborted.store(true);
         return true;
     }
 
@@ -49,7 +49,7 @@ class BasicFolderLoadTask : public Task {
         }
 
         if (m_aborted)
-            emitAborted();
+            emit finished();
         else
             emitSucceeded();
     }
@@ -58,7 +58,7 @@ private:
     QDir m_dir;
     ResultPtr m_result;
 
-    bool m_aborted = false;
+    std::atomic<bool> m_aborted = false;
 
     std::function<Resource*(QFileInfo const&)> m_create_func;
 };

launcher/minecraft/mod/tasks/LocalModParseTask.cpp

@@ -499,7 +499,7 @@ void LocalModParseTask::processAsLitemod()
 
 bool LocalModParseTask::abort()
 {
-    m_aborted = true;
+    m_aborted.store(true);
     return true;
 }
 
@@ -521,7 +521,7 @@ void LocalModParseTask::executeTask()
     }
 
     if (m_aborted)
-        emitAborted();
+        emit finished();
     else
         emitSucceeded();
 }

launcher/minecraft/mod/tasks/LocalModParseTask.h

@@ -39,5 +39,5 @@ private:
     QFileInfo m_modFile;
     ResultPtr m_result;
 
-    bool m_aborted = false;
+    std::atomic<bool> m_aborted = false;
 };

launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp

@@ -38,8 +38,8 @@
 
 #include "minecraft/mod/MetadataHandler.h"
 
-ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan, QObject* parent)
+ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan)
-    : Task(parent, false), m_mods_dir(mods_dir), m_index_dir(index_dir), m_is_indexed(is_indexed), m_clean_orphan(clean_orphan), m_result(new Result())
+    : Task(nullptr, false), m_mods_dir(mods_dir), m_index_dir(index_dir), m_is_indexed(is_indexed), m_clean_orphan(clean_orphan), m_result(new Result())
 {}
 
 void ModFolderLoadTask::executeTask()
@@ -96,6 +96,9 @@ void ModFolderLoadTask::executeTask()
         }
     }
 
+    if (m_aborted)
+        emit finished();
+    else
         emitSucceeded();
 }
 

launcher/minecraft/mod/tasks/ModFolderLoadTask.h

@@ -57,7 +57,15 @@ public:
     }
 
 public:
-    ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan = false, QObject* parent = nullptr);
+    ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan = false);
+
+    [[nodiscard]] bool canAbort() const override { return true; }
+    bool abort() override
+    {
+        m_aborted.store(true);
+        return true;
+    }
+
 
     void executeTask() override;
 
@@ -69,4 +77,6 @@ private:
     bool m_is_indexed;
     bool m_clean_orphan;
     ResultPtr m_result;
+
+    std::atomic<bool> m_aborted = false;
 };