From 369a8cdc7497d7bc335b660ac6c14652abe8fde7 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 22 Jul 2022 00:28:08 -0300 Subject: [PATCH 1/9] fix: only try to start tasks that are really there This fixes an annoying issue where concurrent tasks would try to start multiple tasks even when there was not that many tasks to run in the first place, causing some amount of log spam. Signed-off-by: flow --- launcher/tasks/ConcurrentTask.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/launcher/tasks/ConcurrentTask.cpp b/launcher/tasks/ConcurrentTask.cpp index ab7cbd03..2af296b9 100644 --- a/launcher/tasks/ConcurrentTask.cpp +++ b/launcher/tasks/ConcurrentTask.cpp @@ -37,7 +37,8 @@ void ConcurrentTask::executeTask() { m_total_size = m_queue.size(); - for (int i = 0; i < m_total_max_size; i++) { + int num_starts = std::min(m_total_max_size, m_total_size); + for (int i = 0; i < num_starts; i++) { QMetaObject::invokeMethod(this, &ConcurrentTask::startNext, Qt::QueuedConnection); } } From a720bcc637957ea7d5ed5594797a682818e4bfef Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 22 Jul 2022 00:29:28 -0300 Subject: [PATCH 2/9] fix: bogus progress update when the total step progress was zero Signed-off-by: flow --- launcher/tasks/ConcurrentTask.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/launcher/tasks/ConcurrentTask.cpp b/launcher/tasks/ConcurrentTask.cpp index 2af296b9..ce2f9b87 100644 --- a/launcher/tasks/ConcurrentTask.cpp +++ b/launcher/tasks/ConcurrentTask.cpp @@ -132,11 +132,6 @@ void ConcurrentTask::subTaskStatus(const QString& msg) void ConcurrentTask::subTaskProgress(qint64 current, qint64 total) { - if (total == 0) { - setProgress(0, 100); - return; - } - m_stepProgress = current; m_stepTotalProgress = total; } From c410bb4ecbc553fa6016a4c4e58026343deeec33 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 22 Jul 2022 01:19:56 -0300 Subject: [PATCH 3/9] fix: 'succeeded while not running' spam in ConcurrentTask Signed-off-by: flow --- launcher/tasks/ConcurrentTask.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/tasks/ConcurrentTask.cpp b/launcher/tasks/ConcurrentTask.cpp index ce2f9b87..ecd7ae13 100644 --- a/launcher/tasks/ConcurrentTask.cpp +++ b/launcher/tasks/ConcurrentTask.cpp @@ -71,7 +71,7 @@ void ConcurrentTask::startNext() if (m_aborted || m_doing.count() > m_total_max_size) return; - if (m_queue.isEmpty() && m_doing.isEmpty()) { + if (m_queue.isEmpty() && m_doing.isEmpty() && !wasSuccessful()) { emitSucceeded(); return; } From bdf464e792f4b0c8a20f92a9073699c5fd98cd46 Mon Sep 17 00:00:00 2001 From: flow Date: Sun, 24 Jul 2022 17:16:14 -0300 Subject: [PATCH 4/9] fix: abort logic running subsequent tasks anyways some times Signed-off-by: flow --- launcher/tasks/ConcurrentTask.cpp | 20 +++++++++++++------- launcher/tasks/ConcurrentTask.h | 4 +++- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/launcher/tasks/ConcurrentTask.cpp b/launcher/tasks/ConcurrentTask.cpp index ecd7ae13..484ac58e 100644 --- a/launcher/tasks/ConcurrentTask.cpp +++ b/launcher/tasks/ConcurrentTask.cpp @@ -45,25 +45,31 @@ void ConcurrentTask::executeTask() bool ConcurrentTask::abort() { + m_queue.clear(); + m_aborted = true; + if (m_doing.isEmpty()) { // Don't call emitAborted() here, we want to bypass the 'is the task running' check emit aborted(); emit finished(); - m_aborted = true; return true; } - m_queue.clear(); + bool suceedeed = true; - m_aborted = true; - for (auto task : m_doing) - m_aborted &= task->abort(); + QMutableHashIterator doing_iter(m_doing); + while (doing_iter.hasNext()) { + auto task = doing_iter.next(); + suceedeed &= (task.value())->abort(); + } - if (m_aborted) + if (suceedeed) emitAborted(); + else + emitFailed(tr("Failed to abort all running tasks.")); - return m_aborted; + return suceedeed; } void ConcurrentTask::startNext() diff --git a/launcher/tasks/ConcurrentTask.h b/launcher/tasks/ConcurrentTask.h index 5898899d..f1279d32 100644 --- a/launcher/tasks/ConcurrentTask.h +++ b/launcher/tasks/ConcurrentTask.h @@ -9,7 +9,9 @@ class ConcurrentTask : public Task { Q_OBJECT public: explicit ConcurrentTask(QObject* parent = nullptr, QString task_name = "", int max_concurrent = 6); - virtual ~ConcurrentTask(); + ~ConcurrentTask() override; + + bool canAbort() const override { return true; } inline auto isMultiStep() const -> bool override { return m_queue.size() > 1; }; auto getStepProgress() const -> qint64 override; From e89969991868b05723ae87454d4e22e370137d15 Mon Sep 17 00:00:00 2001 From: flow Date: Sun, 12 Jun 2022 13:30:09 -0300 Subject: [PATCH 5/9] refactor: make SequentialTask inherit from ConcurrentTask In a way, sequential tasks are just concurrent tasks with only a single task running concurrently, so we can remove LOTS of duplicated logic :) Signed-off-by: flow --- launcher/tasks/SequentialTask.cpp | 108 +++--------------------------- launcher/tasks/SequentialTask.h | 58 +++++----------- 2 files changed, 26 insertions(+), 140 deletions(-) diff --git a/launcher/tasks/SequentialTask.cpp b/launcher/tasks/SequentialTask.cpp index f1e1a889..a34137cb 100644 --- a/launcher/tasks/SequentialTask.cpp +++ b/launcher/tasks/SequentialTask.cpp @@ -2,107 +2,21 @@ #include -SequentialTask::SequentialTask(QObject* parent, const QString& task_name) : Task(parent), m_name(task_name), m_currentIndex(-1) {} - -SequentialTask::~SequentialTask() -{ - for(auto task : m_queue){ - if(task) - task->deleteLater(); - } -} - -auto SequentialTask::getStepProgress() const -> qint64 -{ - return m_stepProgress; -} - -auto SequentialTask::getStepTotalProgress() const -> qint64 -{ - return m_stepTotalProgress; -} - -void SequentialTask::addTask(Task::Ptr task) -{ - m_queue.append(task); -} - -void SequentialTask::executeTask() -{ - m_currentIndex = -1; - startNext(); -} - -bool SequentialTask::abort() -{ - if(m_currentIndex == -1 || m_currentIndex >= m_queue.size()) { - if(m_currentIndex == -1) { - // Don't call emitAborted() here, we want to bypass the 'is the task running' check - emit aborted(); - emit finished(); - } - - m_aborted = true; - return true; - } - - bool succeeded = m_queue[m_currentIndex]->abort(); - m_aborted = succeeded; - - if (succeeded) - emitAborted(); - - return succeeded; -} +SequentialTask::SequentialTask(QObject* parent, QString task_name) : ConcurrentTask(parent, task_name, 1) {} void SequentialTask::startNext() { - if (m_aborted) - return; - - if (m_currentIndex != -1 && m_currentIndex < m_queue.size()) { - Task::Ptr previous = m_queue.at(m_currentIndex); - disconnect(previous.get(), 0, this, 0); - } - - m_currentIndex++; - if (m_queue.isEmpty() || m_currentIndex >= m_queue.size()) { - emitSucceeded(); - return; - } - Task::Ptr next = m_queue[m_currentIndex]; - - connect(next.get(), SIGNAL(failed(QString)), this, SLOT(subTaskFailed(QString))); - connect(next.get(), SIGNAL(succeeded()), this, SLOT(startNext())); - - connect(next.get(), SIGNAL(status(QString)), this, SLOT(subTaskStatus(QString))); - connect(next.get(), SIGNAL(stepStatus(QString)), this, SLOT(subTaskStatus(QString))); - - connect(next.get(), SIGNAL(progress(qint64, qint64)), this, SLOT(subTaskProgress(qint64, qint64))); - - setStatus(tr("Executing task %1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size())); - setStepStatus(next->isMultiStep() ? next->getStepStatus() : next->getStatus()); - - setProgress(m_currentIndex + 1, m_queue.count()); - - next->start(); -} - -void SequentialTask::subTaskFailed(const QString& msg) -{ - emitFailed(msg); -} -void SequentialTask::subTaskStatus(const QString& msg) -{ - setStepStatus(msg); -} -void SequentialTask::subTaskProgress(qint64 current, qint64 total) -{ - if (total == 0) { - setProgress(0, 100); + if (m_failed.size() > 0) { + emitFailed(tr("One of the tasks failed!")); + qWarning() << m_failed.constBegin()->get()->failReason(); return; } - m_stepProgress = current; - m_stepTotalProgress = total; + ConcurrentTask::startNext(); +} + +void SequentialTask::updateState() +{ + setProgress(m_done.count(), m_total_size); + setStatus(tr("Executing task %1 out of %2").arg(QString::number(m_doing.count() + m_done.count()), QString::number(m_total_size))); } diff --git a/launcher/tasks/SequentialTask.h b/launcher/tasks/SequentialTask.h index f5a58b1b..5eace96e 100644 --- a/launcher/tasks/SequentialTask.h +++ b/launcher/tasks/SequentialTask.h @@ -1,49 +1,21 @@ #pragma once -#include "Task.h" -#include "QObjectPtr.h" +#include "ConcurrentTask.h" -#include - -class SequentialTask : public Task -{ +/** A concurrent task that only allows one concurrent task :) + * + * This should be used when there's a need to maintain a strict ordering of task executions, and + * the starting of a task is contingent on the success of the previous one. + * + * See MultipleOptionsTask if that's not the case. + */ +class SequentialTask : public ConcurrentTask { Q_OBJECT -public: - explicit SequentialTask(QObject *parent = nullptr, const QString& task_name = ""); - virtual ~SequentialTask(); + public: + explicit SequentialTask(QObject* parent = nullptr, QString task_name = ""); + ~SequentialTask() override = default; - inline auto isMultiStep() const -> bool override { return m_queue.size() > 1; }; - auto getStepProgress() const -> qint64 override; - auto getStepTotalProgress() const -> qint64 override; - - inline auto getStepStatus() const -> QString override { return m_step_status; } - - void addTask(Task::Ptr task); - -public slots: - bool abort() override; - -protected -slots: - void executeTask() override; - - virtual void startNext(); - virtual void subTaskFailed(const QString &msg); - virtual void subTaskStatus(const QString &msg); - virtual void subTaskProgress(qint64 current, qint64 total); - -protected: - void setStepStatus(QString status) { m_step_status = status; emit stepStatus(status); }; - -protected: - QString m_name; - QString m_step_status; - - QQueue m_queue; - int m_currentIndex; - - qint64 m_stepProgress = 0; - qint64 m_stepTotalProgress = 100; - - bool m_aborted = false; + protected: + void startNext() override; + void updateState() override; }; From 87a0482b8b299fd54691e3042ca661863ea6290a Mon Sep 17 00:00:00 2001 From: flow Date: Thu, 21 Jul 2022 22:40:06 -0300 Subject: [PATCH 6/9] refactor: make MultipleOptionsTask inherit from ConcurrentTask too Signed-off-by: flow --- launcher/tasks/MultipleOptionsTask.cpp | 45 +++++++------------------- launcher/tasks/MultipleOptionsTask.h | 16 ++++----- 2 files changed, 19 insertions(+), 42 deletions(-) diff --git a/launcher/tasks/MultipleOptionsTask.cpp b/launcher/tasks/MultipleOptionsTask.cpp index 6e853568..7741bef0 100644 --- a/launcher/tasks/MultipleOptionsTask.cpp +++ b/launcher/tasks/MultipleOptionsTask.cpp @@ -2,47 +2,26 @@ #include -MultipleOptionsTask::MultipleOptionsTask(QObject* parent, const QString& task_name) : SequentialTask(parent, task_name) {} +MultipleOptionsTask::MultipleOptionsTask(QObject* parent, const QString& task_name) : ConcurrentTask(parent, task_name) {} void MultipleOptionsTask::startNext() { - Task* previous = nullptr; - if (m_currentIndex != -1) { - previous = m_queue[m_currentIndex].get(); - disconnect(previous, 0, this, 0); - } - - m_currentIndex++; - if ((previous && previous->wasSuccessful())) { + if (m_done.size() != m_failed.size()) { emitSucceeded(); return; } - Task::Ptr next = m_queue[m_currentIndex]; - - connect(next.get(), &Task::failed, this, &MultipleOptionsTask::subTaskFailed); - connect(next.get(), &Task::succeeded, this, &MultipleOptionsTask::startNext); - - connect(next.get(), &Task::status, this, &MultipleOptionsTask::subTaskStatus); - connect(next.get(), &Task::stepStatus, this, &MultipleOptionsTask::subTaskStatus); - - connect(next.get(), &Task::progress, this, &MultipleOptionsTask::subTaskProgress); - - qDebug() << QString("Making attemp %1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size()); - setStatus(tr("Making attempt #%1 out of %2").arg(m_currentIndex + 1).arg(m_queue.size())); - setStepStatus(next->isMultiStep() ? next->getStepStatus() : next->getStatus()); - - next->start(); -} - -void MultipleOptionsTask::subTaskFailed(QString const& reason) -{ - qDebug() << QString("Failed attempt #%1 of %2. Reason: %3").arg(m_currentIndex + 1).arg(m_queue.size()).arg(reason); - if(m_currentIndex < m_queue.size() - 1) { - startNext(); + if (m_queue.isEmpty()) { + emitFailed(tr("All attempts have failed!")); + qWarning() << "All attempts have failed!"; return; } - qWarning() << QString("All attempts have failed!"); - emitFailed(); + ConcurrentTask::startNext(); +} + +void MultipleOptionsTask::updateState() +{ + setProgress(m_done.count(), m_total_size); + setStatus(tr("Attempting task %1 out of %2").arg(QString::number(m_doing.count() + m_done.count()), QString::number(m_total_size))); } diff --git a/launcher/tasks/MultipleOptionsTask.h b/launcher/tasks/MultipleOptionsTask.h index 7c508b00..c65356b0 100644 --- a/launcher/tasks/MultipleOptionsTask.h +++ b/launcher/tasks/MultipleOptionsTask.h @@ -1,19 +1,17 @@ #pragma once -#include "SequentialTask.h" +#include "ConcurrentTask.h" /* This task type will attempt to do run each of it's subtasks in sequence, * until one of them succeeds. When that happens, the remaining tasks will not run. * */ -class MultipleOptionsTask : public SequentialTask -{ +class MultipleOptionsTask : public ConcurrentTask { Q_OBJECT -public: - explicit MultipleOptionsTask(QObject *parent = nullptr, const QString& task_name = ""); - virtual ~MultipleOptionsTask() = default; + public: + explicit MultipleOptionsTask(QObject* parent = nullptr, const QString& task_name = ""); + ~MultipleOptionsTask() override = default; -private -slots: + private slots: void startNext() override; - void subTaskFailed(const QString &msg) override; + void updateState() override; }; From 7b6d26990469f26dfbed6d55af25240ef0618df4 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 22 Jul 2022 00:30:27 -0300 Subject: [PATCH 7/9] refactor: make NetJob inherit from ConcurrentTask as well! Avoids lots of code duplication Signed-off-by: flow --- launcher/net/NetAction.h | 2 + launcher/net/NetJob.cpp | 202 ++++-------------- launcher/net/NetJob.h | 48 ++--- launcher/ui/pages/modplatform/ModModel.cpp | 5 +- .../modplatform/modrinth/ModrinthModel.cpp | 5 +- 5 files changed, 64 insertions(+), 198 deletions(-) diff --git a/launcher/net/NetAction.h b/launcher/net/NetAction.h index 729d4132..d9c4fadc 100644 --- a/launcher/net/NetAction.h +++ b/launcher/net/NetAction.h @@ -54,6 +54,8 @@ class NetAction : public Task { QUrl url() { return m_url; } auto index() -> int { return m_index_within_job; } + void setNetwork(shared_qobject_ptr network) { m_network = network; } + protected slots: virtual void downloadProgress(qint64 bytesReceived, qint64 bytesTotal) = 0; virtual void downloadError(QNetworkReply::NetworkError error) = 0; diff --git a/launcher/net/NetJob.cpp b/launcher/net/NetJob.cpp index bab35fa5..20d75976 100644 --- a/launcher/net/NetJob.cpp +++ b/launcher/net/NetJob.cpp @@ -35,204 +35,90 @@ */ #include "NetJob.h" -#include "Download.h" auto NetJob::addNetAction(NetAction::Ptr action) -> bool { - action->m_index_within_job = m_downloads.size(); - m_downloads.append(action); - part_info pi; - m_parts_progress.append(pi); + action->m_index_within_job = m_queue.size(); + m_queue.append(action); - partProgress(m_parts_progress.count() - 1, action->getProgress(), action->getTotalProgress()); - - if (action->isRunning()) { - connect(action.get(), &NetAction::succeeded, [this, action]{ partSucceeded(action->index()); }); - connect(action.get(), &NetAction::failed, [this, action](QString){ partFailed(action->index()); }); - connect(action.get(), &NetAction::aborted, [this, action](){ partAborted(action->index()); }); - connect(action.get(), &NetAction::progress, [this, action](qint64 done, qint64 total) { partProgress(action->index(), done, total); }); - connect(action.get(), &NetAction::status, this, &NetJob::status); - } else { - m_todo.append(m_parts_progress.size() - 1); - } + action->setNetwork(m_network); return true; } +void NetJob::startNext() +{ + if (m_queue.isEmpty() && m_doing.isEmpty()) { + // We're finished, check for failures and retry if we can (up to 3 times) + if (!m_failed.isEmpty() && m_try < 3) { + m_try += 1; + while (!m_failed.isEmpty()) + m_queue.enqueue(m_failed.take(*m_failed.keyBegin())); + } + } + + ConcurrentTask::startNext(); +} + +auto NetJob::size() const -> int +{ + return m_queue.size() + m_doing.size() + m_done.size(); +} + auto NetJob::canAbort() const -> bool { bool canFullyAbort = true; // can abort the downloads on the queue? - for (auto index : m_todo) { - auto part = m_downloads[index]; + for (auto part : m_queue) canFullyAbort &= part->canAbort(); - } + // can abort the active downloads? - for (auto index : m_doing) { - auto part = m_downloads[index]; + for (auto part : m_doing) canFullyAbort &= part->canAbort(); - } return canFullyAbort; } -void NetJob::executeTask() -{ - // hack that delays early failures so they can be caught easier - QMetaObject::invokeMethod(this, "startMoreParts", Qt::QueuedConnection); -} - -auto NetJob::getFailedFiles() -> QStringList -{ - QStringList failed; - for (auto index : m_failed) { - failed.push_back(m_downloads[index]->url().toString()); - } - failed.sort(); - return failed; -} - auto NetJob::abort() -> bool { bool fullyAborted = true; // fail all downloads on the queue -#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0) - QSet todoSet(m_todo.begin(), m_todo.end()); - m_failed.unite(todoSet); -#else - m_failed.unite(m_todo.toSet()); -#endif - m_todo.clear(); + for (auto task : m_queue) + m_failed.insert(task.get(), task); + m_queue.clear(); // abort active downloads auto toKill = m_doing.values(); - for (auto index : toKill) { - auto part = m_downloads[index]; + for (auto part : toKill) { fullyAborted &= part->abort(); } return fullyAborted; } -void NetJob::partSucceeded(int index) +auto NetJob::getFailedActions() -> QList { - // do progress. all slots are 1 in size at least - auto& slot = m_parts_progress[index]; - partProgress(index, slot.total_progress, slot.total_progress); - - m_doing.remove(index); - m_done.insert(index); - m_downloads[index].get()->disconnect(this); - - startMoreParts(); + QList failed; + for (auto index : m_failed) { + failed.push_back(dynamic_cast(index.get())); + } + return failed; } -void NetJob::partFailed(int index) +auto NetJob::getFailedFiles() -> QList { - m_doing.remove(index); - - auto& slot = m_parts_progress[index]; - // Can try 3 times before failing by definitive - if (slot.failures == 3) { - m_failed.insert(index); - } else { - slot.failures++; - m_todo.enqueue(index); + QList failed; + for (auto index : m_failed) { + failed.append(static_cast(index.get())->url().toString()); } - - m_downloads[index].get()->disconnect(this); - - startMoreParts(); + return failed; } -void NetJob::partAborted(int index) +void NetJob::updateState() { - m_aborted = true; - - m_doing.remove(index); - m_failed.insert(index); - m_downloads[index].get()->disconnect(this); - - startMoreParts(); -} - -void NetJob::partProgress(int index, qint64 bytesReceived, qint64 bytesTotal) -{ - auto& slot = m_parts_progress[index]; - slot.current_progress = bytesReceived; - slot.total_progress = bytesTotal; - - int done = m_done.size(); - int doing = m_doing.size(); - int all = m_parts_progress.size(); - - qint64 bytesAll = 0; - qint64 bytesTotalAll = 0; - for (auto& partIdx : m_doing) { - auto part = m_parts_progress[partIdx]; - // do not count parts with unknown/nonsensical total size - if (part.total_progress <= 0) { - continue; - } - bytesAll += part.current_progress; - bytesTotalAll += part.total_progress; - } - - qint64 inprogress = (bytesTotalAll == 0) ? 0 : (bytesAll * 1000) / bytesTotalAll; - auto current = done * 1000 + doing * inprogress; - auto current_total = all * 1000; - // HACK: make sure it never jumps backwards. - // FAIL: This breaks if the size is not known (or is it something else?) and jumps to 1000, so if it is 1000 reset it to inprogress - if (m_current_progress == 1000) { - m_current_progress = inprogress; - } - if (m_current_progress > current) { - current = m_current_progress; - } - m_current_progress = current; - setProgress(current, current_total); -} - -void NetJob::startMoreParts() -{ - if (!isRunning()) { - // this actually makes sense. You can put running m_downloads into a NetJob and then not start it until much later. - return; - } - - // OK. We are actively processing tasks, proceed. - // Check for final conditions if there's nothing in the queue. - if (!m_todo.size()) { - if (!m_doing.size()) { - if (!m_failed.size()) { - emitSucceeded(); - } else if (m_aborted) { - emitAborted(); - } else { - emitFailed(tr("Job '%1' failed to process:\n%2").arg(objectName()).arg(getFailedFiles().join("\n"))); - } - } - return; - } - - // There's work to do, try to start more parts, to a maximum of 6 concurrent ones. - while (m_doing.size() < 6) { - if (m_todo.size() == 0) - return; - int doThis = m_todo.dequeue(); - m_doing.insert(doThis); - - auto part = m_downloads[doThis]; - - // connect signals :D - connect(part.get(), &NetAction::succeeded, this, [this, part]{ partSucceeded(part->index()); }); - connect(part.get(), &NetAction::failed, this, [this, part](QString){ partFailed(part->index()); }); - connect(part.get(), &NetAction::aborted, this, [this, part]{ partAborted(part->index()); }); - connect(part.get(), &NetAction::progress, this, [this, part](qint64 done, qint64 total) { partProgress(part->index(), done, total); }); - connect(part.get(), &NetAction::status, this, &NetJob::status); - - part->startAction(m_network); - } + emit progress(m_done.count(), m_total_size); + setStatus(tr("Executing %1 task(s) (%2 out of %3 are done)") + .arg(QString::number(m_doing.count()), QString::number(m_done.count()), QString::number(m_total_size))); } diff --git a/launcher/net/NetJob.h b/launcher/net/NetJob.h index 63c1cf51..cd5d5e48 100644 --- a/launcher/net/NetJob.h +++ b/launcher/net/NetJob.h @@ -39,64 +39,40 @@ #include #include "NetAction.h" -#include "tasks/Task.h" +#include "tasks/ConcurrentTask.h" // Those are included so that they are also included by anyone using NetJob #include "net/Download.h" #include "net/HttpMetaCache.h" -class NetJob : public Task { +class NetJob : public ConcurrentTask { Q_OBJECT public: using Ptr = shared_qobject_ptr; - explicit NetJob(QString job_name, shared_qobject_ptr network) : Task(), m_network(network) - { - setObjectName(job_name); - } - virtual ~NetJob() = default; + explicit NetJob(QString job_name, shared_qobject_ptr network) : ConcurrentTask(nullptr, job_name), m_network(network) {} + ~NetJob() override = default; - void executeTask() override; + void startNext() override; + + auto size() const -> int; auto canAbort() const -> bool override; - auto addNetAction(NetAction::Ptr action) -> bool; - auto operator[](int index) -> NetAction::Ptr { return m_downloads[index]; } - auto at(int index) -> const NetAction::Ptr { return m_downloads.at(index); } - auto size() const -> int { return m_downloads.size(); } - auto first() -> NetAction::Ptr { return m_downloads.size() != 0 ? m_downloads[0] : NetAction::Ptr{}; } - - auto getFailedFiles() -> QStringList; + auto getFailedActions() -> QList; + auto getFailedFiles() -> QList; public slots: // Qt can't handle auto at the start for some reason? bool abort() override; - private slots: - void startMoreParts(); - - void partProgress(int index, qint64 bytesReceived, qint64 bytesTotal); - void partSucceeded(int index); - void partFailed(int index); - void partAborted(int index); + protected: + void updateState() override; private: shared_qobject_ptr m_network; - struct part_info { - qint64 current_progress = 0; - qint64 total_progress = 1; - int failures = 0; - }; - - QList m_downloads; - QList m_parts_progress; - QQueue m_todo; - QSet m_doing; - QSet m_done; - QSet m_failed; - qint64 m_current_progress = 0; - bool m_aborted = false; + int m_try = 1; }; diff --git a/launcher/ui/pages/modplatform/ModModel.cpp b/launcher/ui/pages/modplatform/ModModel.cpp index 94b1f099..06a6f6b8 100644 --- a/launcher/ui/pages/modplatform/ModModel.cpp +++ b/launcher/ui/pages/modplatform/ModModel.cpp @@ -230,10 +230,11 @@ void ListModel::searchRequestFinished(QJsonDocument& doc) void ListModel::searchRequestFailed(QString reason) { - if (!jobPtr->first()->m_reply) { + auto failed_action = jobPtr->getFailedActions().at(0); + if (!failed_action->m_reply) { // Network error QMessageBox::critical(nullptr, tr("Error"), tr("A network error occurred. Could not load mods.")); - } else if (jobPtr->first()->m_reply && jobPtr->first()->m_reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 409) { + } else if (failed_action->m_reply && failed_action->m_reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 409) { // 409 Gone, notify user to update QMessageBox::critical(nullptr, tr("Error"), //: %1 refers to the launcher itself diff --git a/launcher/ui/pages/modplatform/modrinth/ModrinthModel.cpp b/launcher/ui/pages/modplatform/modrinth/ModrinthModel.cpp index 3633d575..614be434 100644 --- a/launcher/ui/pages/modplatform/modrinth/ModrinthModel.cpp +++ b/launcher/ui/pages/modplatform/modrinth/ModrinthModel.cpp @@ -301,10 +301,11 @@ void ModpackListModel::searchRequestFinished(QJsonDocument& doc_all) void ModpackListModel::searchRequestFailed(QString reason) { - if (!jobPtr->first()->m_reply) { + auto failed_action = jobPtr->getFailedActions().at(0); + if (!failed_action->m_reply) { // Network error QMessageBox::critical(nullptr, tr("Error"), tr("A network error occurred. Could not load modpacks.")); - } else if (jobPtr->first()->m_reply && jobPtr->first()->m_reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 409) { + } else if (failed_action->m_reply && failed_action->m_reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 409) { // 409 Gone, notify user to update QMessageBox::critical(nullptr, tr("Error"), //: %1 refers to the launcher itself From 247f99ce2f981adad48974abf95ef5e10b832503 Mon Sep 17 00:00:00 2001 From: flow Date: Fri, 22 Jul 2022 01:21:57 -0300 Subject: [PATCH 8/9] feat(test): add more tests to Tasks Signed-off-by: flow --- launcher/tasks/Task_test.cpp | 125 ++++++++++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 1 deletion(-) diff --git a/launcher/tasks/Task_test.cpp b/launcher/tasks/Task_test.cpp index ef153a6a..b56ee8a6 100644 --- a/launcher/tasks/Task_test.cpp +++ b/launcher/tasks/Task_test.cpp @@ -1,5 +1,8 @@ #include +#include "ConcurrentTask.h" +#include "MultipleOptionsTask.h" +#include "SequentialTask.h" #include "Task.h" /* Does nothing. Only used for testing. */ @@ -9,7 +12,10 @@ class BasicTask : public Task { friend class TaskTest; private: - void executeTask() override {}; + void executeTask() override + { + emitSucceeded(); + }; }; /* Does nothing. Only used for testing. */ @@ -60,6 +66,123 @@ class TaskTest : public QObject { QCOMPARE(t.getProgress(), current); QCOMPARE(t.getTotalProgress(), total); } + + void test_basicRun(){ + BasicTask t; + QObject::connect(&t, &Task::finished, [&]{ QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); }); + t.start(); + + QVERIFY2(QTest::qWaitFor([&]() { + return t.isFinished(); + }, 1000), "Task didn't finish as it should."); + } + + void test_basicConcurrentRun(){ + BasicTask t1; + BasicTask t2; + BasicTask t3; + + ConcurrentTask t; + + t.addTask(&t1); + t.addTask(&t2); + t.addTask(&t3); + + QObject::connect(&t, &Task::finished, [&]{ + QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); + QVERIFY(t1.wasSuccessful()); + QVERIFY(t2.wasSuccessful()); + QVERIFY(t3.wasSuccessful()); + }); + + t.start(); + QVERIFY2(QTest::qWaitFor([&]() { + return t.isFinished(); + }, 1000), "Task didn't finish as it should."); + } + + // Tests if starting new tasks after the 6 initial ones is working + void test_moreConcurrentRun(){ + BasicTask t1, t2, t3, t4, t5, t6, t7, t8, t9; + + ConcurrentTask t; + + t.addTask(&t1); + t.addTask(&t2); + t.addTask(&t3); + t.addTask(&t4); + t.addTask(&t5); + t.addTask(&t6); + t.addTask(&t7); + t.addTask(&t8); + t.addTask(&t9); + + QObject::connect(&t, &Task::finished, [&]{ + QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); + QVERIFY(t1.wasSuccessful()); + QVERIFY(t2.wasSuccessful()); + QVERIFY(t3.wasSuccessful()); + QVERIFY(t4.wasSuccessful()); + QVERIFY(t5.wasSuccessful()); + QVERIFY(t6.wasSuccessful()); + QVERIFY(t7.wasSuccessful()); + QVERIFY(t8.wasSuccessful()); + QVERIFY(t9.wasSuccessful()); + }); + + t.start(); + QVERIFY2(QTest::qWaitFor([&]() { + return t.isFinished(); + }, 1000), "Task didn't finish as it should."); + } + + void test_basicSequentialRun(){ + BasicTask t1; + BasicTask t2; + BasicTask t3; + + SequentialTask t; + + t.addTask(&t1); + t.addTask(&t2); + t.addTask(&t3); + + QObject::connect(&t, &Task::finished, [&]{ + QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); + QVERIFY(t1.wasSuccessful()); + QVERIFY(t2.wasSuccessful()); + QVERIFY(t3.wasSuccessful()); + }); + + t.start(); + QVERIFY2(QTest::qWaitFor([&]() { + return t.isFinished(); + }, 1000), "Task didn't finish as it should."); + } + + void test_basicMultipleOptionsRun(){ + BasicTask t1; + BasicTask t2; + BasicTask t3; + + MultipleOptionsTask t; + + t.addTask(&t1); + t.addTask(&t2); + t.addTask(&t3); + + QObject::connect(&t, &Task::finished, [&]{ + QVERIFY2(t.wasSuccessful(), "Task finished but was not successful when it should have been."); + QVERIFY(t1.wasSuccessful()); + QVERIFY(!t2.wasSuccessful()); + QVERIFY(!t3.wasSuccessful()); + }); + + t.start(); + QVERIFY2(QTest::qWaitFor([&]() { + return t.isFinished(); + }, 1000), "Task didn't finish as it should."); + } }; QTEST_GUILESS_MAIN(TaskTest) From 064ae49d2b227bbc64c687e2d05fc0554ada6a31 Mon Sep 17 00:00:00 2001 From: flow Date: Sun, 28 Aug 2022 15:51:14 -0300 Subject: [PATCH 9/9] fix: make MultipleOptionsTask inherit directly from SequentialTask It's not a good idea to have multiple concurrent tasks running on a sequential thing like this one. Signed-off-by: flow --- launcher/tasks/MultipleOptionsTask.cpp | 2 +- launcher/tasks/MultipleOptionsTask.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/launcher/tasks/MultipleOptionsTask.cpp b/launcher/tasks/MultipleOptionsTask.cpp index 7741bef0..5ad6181f 100644 --- a/launcher/tasks/MultipleOptionsTask.cpp +++ b/launcher/tasks/MultipleOptionsTask.cpp @@ -2,7 +2,7 @@ #include -MultipleOptionsTask::MultipleOptionsTask(QObject* parent, const QString& task_name) : ConcurrentTask(parent, task_name) {} +MultipleOptionsTask::MultipleOptionsTask(QObject* parent, const QString& task_name) : SequentialTask(parent, task_name) {} void MultipleOptionsTask::startNext() { diff --git a/launcher/tasks/MultipleOptionsTask.h b/launcher/tasks/MultipleOptionsTask.h index c65356b0..db7d4d9a 100644 --- a/launcher/tasks/MultipleOptionsTask.h +++ b/launcher/tasks/MultipleOptionsTask.h @@ -1,11 +1,11 @@ #pragma once -#include "ConcurrentTask.h" +#include "SequentialTask.h" /* This task type will attempt to do run each of it's subtasks in sequence, * until one of them succeeds. When that happens, the remaining tasks will not run. * */ -class MultipleOptionsTask : public ConcurrentTask { +class MultipleOptionsTask : public SequentialTask { Q_OBJECT public: explicit MultipleOptionsTask(QObject* parent = nullptr, const QString& task_name = "");