From 42c81395b362b9a080dca3f9d62e33c547cb8259 Mon Sep 17 00:00:00 2001 From: flow Date: Sat, 3 Sep 2022 13:25:05 -0300 Subject: [PATCH] fix: race condition on ResourceFolderModel tests This (hopefully) fixes the race contiditions that sometimes got triggered in tests. Signed-off-by: flow --- launcher/minecraft/mod/ModFolderModel.cpp | 11 +----- .../minecraft/mod/ResourceFolderModel.cpp | 36 ++++++++++--------- launcher/minecraft/mod/ResourceFolderModel.h | 4 +-- .../mod/ResourceFolderModel_test.cpp | 17 +-------- .../minecraft/mod/tasks/BasicFolderLoadTask.h | 6 ++-- .../minecraft/mod/tasks/LocalModParseTask.cpp | 4 +-- .../minecraft/mod/tasks/LocalModParseTask.h | 2 +- .../minecraft/mod/tasks/ModFolderLoadTask.cpp | 9 +++-- .../minecraft/mod/tasks/ModFolderLoadTask.h | 12 ++++++- 9 files changed, 46 insertions(+), 55 deletions(-) diff --git a/launcher/minecraft/mod/ModFolderModel.cpp b/launcher/minecraft/mod/ModFolderModel.cpp index c014742c..13fed1c9 100644 --- a/launcher/minecraft/mod/ModFolderModel.cpp +++ b/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) diff --git a/launcher/minecraft/mod/ResourceFolderModel.cpp b/launcher/minecraft/mod/ResourceFolderModel.cpp index bc18ddc2..45d1db59 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.cpp +++ b/launcher/minecraft/mod/ResourceFolderModel.cpp @@ -1,5 +1,6 @@ #include "ResourceFolderModel.h" +#include #include #include #include @@ -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(); - thread_pool->start(m_current_update_task.get()); + QThreadPool::globalInstance()->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; - m_next_resolution_ticket += 1; - m_ticket_mutex.unlock(); + int ticket = m_next_resolution_ticket.fetch_add(1); 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(); - thread_pool->start(task); + QThreadPool::globalInstance()->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) diff --git a/launcher/minecraft/mod/ResourceFolderModel.h b/launcher/minecraft/mod/ResourceFolderModel.h index a45d1cbd..d763bec3 100644 --- a/launcher/minecraft/mod/ResourceFolderModel.h +++ b/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 m_resources_index; QMap m_active_parse_tasks; - int m_next_resolution_ticket = 0; - QMutex m_ticket_mutex; + std::atomic m_next_resolution_ticket = 0; }; /* A macro to define useful functions to handle Resource* -> T* more easily on derived classes */ diff --git a/launcher/minecraft/mod/ResourceFolderModel_test.cpp b/launcher/minecraft/mod/ResourceFolderModel_test.cpp index 8fe93c2a..aa78e502 100644 --- a/launcher/minecraft/mod/ResourceFolderModel_test.cpp +++ b/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(); - } } }; diff --git a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h index 7ea512b3..be0e752d 100644 --- a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h +++ b/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 m_aborted = false; std::function m_create_func; }; diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp b/launcher/minecraft/mod/tasks/LocalModParseTask.cpp index c486bd46..8a6e54d8 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.cpp +++ b/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(); } diff --git a/launcher/minecraft/mod/tasks/LocalModParseTask.h b/launcher/minecraft/mod/tasks/LocalModParseTask.h index 4bbf3c85..413eb2d1 100644 --- a/launcher/minecraft/mod/tasks/LocalModParseTask.h +++ b/launcher/minecraft/mod/tasks/LocalModParseTask.h @@ -39,5 +39,5 @@ private: QFileInfo m_modFile; ResultPtr m_result; - bool m_aborted = false; + std::atomic m_aborted = false; }; diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index a56ba8ab..3a857740 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/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) - : 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()) +ModFolderLoadTask::ModFolderLoadTask(QDir mods_dir, QDir index_dir, bool is_indexed, bool clean_orphan) + : 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,7 +96,10 @@ void ModFolderLoadTask::executeTask() } } - emitSucceeded(); + if (m_aborted) + emit finished(); + else + emitSucceeded(); } void ModFolderLoadTask::getFromMetadata() diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h b/launcher/minecraft/mod/tasks/ModFolderLoadTask.h index 840e95e1..0f18b8b9 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.h +++ b/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 m_aborted = false; };