From b61407a75d06abd61ce89f972581fa36a961d906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Mr=C3=A1zek?= Date: Tue, 5 Sep 2017 23:38:17 +0200 Subject: [PATCH] NOISSUE retry committing instances if it fails a few times This should fix issues with antivirus locking files/folders on Windows. --- api/logic/FolderInstanceProvider.cpp | 122 +++++++++++++++++++++------ api/logic/InstanceCopyTask.cpp | 9 +- api/logic/InstanceCopyTask.h | 3 +- api/logic/InstanceCreationTask.cpp | 30 +++---- api/logic/InstanceCreationTask.h | 6 +- api/logic/InstanceImportTask.cpp | 44 +--------- api/logic/InstanceImportTask.h | 3 +- 7 files changed, 121 insertions(+), 96 deletions(-) diff --git a/api/logic/FolderInstanceProvider.cpp b/api/logic/FolderInstanceProvider.cpp index 4893efd8..e0b578e8 100644 --- a/api/logic/FolderInstanceProvider.cpp +++ b/api/logic/FolderInstanceProvider.cpp @@ -12,6 +12,7 @@ #include #include #include +#include const static int GROUP_FILE_FORMAT_VERSION = 1; @@ -110,24 +111,6 @@ InstancePtr FolderInstanceProvider::loadInstance(const InstanceId& id) return inst; } -#include "InstanceImportTask.h" -Task * FolderInstanceProvider::zipImportTask(const QUrl sourceUrl, const QString& instName, const QString& instGroup, const QString& instIcon) -{ - return new InstanceImportTask(m_globalSettings, sourceUrl, this, instName, instIcon, instGroup); -} - -#include "InstanceCreationTask.h" -Task * FolderInstanceProvider::creationTask(BaseVersionPtr version, const QString& instName, const QString& instGroup, const QString& instIcon) -{ - return new InstanceCreationTask(m_globalSettings, this, version, instName, instIcon, instGroup); -} - -#include "InstanceCopyTask.h" -Task * FolderInstanceProvider::copyTask(const InstancePtr& oldInstance, const QString& instName, const QString& instGroup, const QString& instIcon, bool copySaves) -{ - return new InstanceCopyTask(m_globalSettings, this, oldInstance, instName, instIcon, instGroup, copySaves); -} - void FolderInstanceProvider::saveGroupList() { WatchLock foo(m_watcher, m_instDir); @@ -310,12 +293,65 @@ void FolderInstanceProvider::on_InstFolderChanged(const Setting &setting, QVaria emit instancesChanged(); } } + +template +static void clamp(T& current, T min, T max) +{ + if (current < min) + { + current = min; + } + else if(current > max) + { + current = max; + } +} + +// List of numbers from min to max. Next is exponent times bigger than previous. +class ExponentialSeries +{ +public: + ExponentialSeries(unsigned min, unsigned max, unsigned exponent = 2) + { + m_current = m_min = min; + m_max = max; + m_exponent = exponent; + } + void reset() + { + m_current = m_min; + } + unsigned operator()() + { + unsigned retval = m_current; + m_current *= m_exponent; + clamp(m_current, m_min, m_max); + return retval; + } + unsigned m_current; + unsigned m_min; + unsigned m_max; + unsigned m_exponent; +}; + /* + * WHY: the whole reason why this uses an exponential backoff retry scheme is antivirus on Windows. + * Basically, it starts messing things up while MultiMC is extracting/creating instances + * and causes that horrible failure that is NTFS to lock files in place because they are open. + */ class FolderInstanceStaging : public Task { - +Q_OBJECT + const unsigned minBackoff = 1; + const unsigned maxBackoff = 16; public: - FolderInstanceStaging(FolderInstanceProvider * parent, Task * child, const QString& instanceName, const QString& groupName) + FolderInstanceStaging ( + FolderInstanceProvider * parent, + Task * child, + const QString & stagingPath, + const QString& instanceName, + const QString& groupName ) + : backoff(minBackoff, maxBackoff) { m_parent = parent; m_child.reset(child); @@ -325,22 +361,34 @@ public: connect(child, &Task::progress, this, &FolderInstanceStaging::setProgress); m_instanceName = instanceName; m_groupName = groupName; + m_stagingPath = stagingPath; + m_backoffTimer.setSingleShot(true); + connect(&m_backoffTimer, &QTimer::timeout, this, &FolderInstanceStaging::childSucceded); } protected: virtual void executeTask() override { - m_stagingPath = m_parent->getStagedInstancePath(); m_child->start(); } private slots: void childSucceded() { + unsigned sleepTime = backoff(); if(m_parent->commitStagedInstance(m_stagingPath, m_instanceName, m_groupName)) + { emitSucceeded(); - // TODO: implement exponential backoff retry scheme with limit - emitFailed("Failed to commit instance"); + return; + } + // we actually failed, retry? + if(sleepTime == maxBackoff) + { + emitFailed(tr("Failed to commit instance, even after multiple retries. It is being blocked by something.")); + return; + } + qDebug() << "Failed to commit instance" << m_instanceName << "Initiating backoff:" << sleepTime; + m_backoffTimer.start(sleepTime * 500); } void childFailed(const QString & reason) { @@ -349,13 +397,38 @@ private slots: } private: + ExponentialSeries backoff; QString m_stagingPath; FolderInstanceProvider * m_parent; unique_qobject_ptr m_child; QString m_instanceName; QString m_groupName; + QTimer m_backoffTimer; }; -*/ + +#include "InstanceImportTask.h" +Task * FolderInstanceProvider::zipImportTask(const QUrl sourceUrl, const QString& instName, const QString& instGroup, const QString& instIcon) +{ + auto stagingPath = getStagedInstancePath(); + auto task = new InstanceImportTask(m_globalSettings, sourceUrl, stagingPath, instName, instIcon, instGroup); + return new FolderInstanceStaging(this, task, stagingPath, instName, instGroup); +} + +#include "InstanceCreationTask.h" +Task * FolderInstanceProvider::creationTask(BaseVersionPtr version, const QString& instName, const QString& instGroup, const QString& instIcon) +{ + auto stagingPath = getStagedInstancePath(); + auto task = new InstanceCreationTask(m_globalSettings, stagingPath, version, instName, instIcon, instGroup); + return new FolderInstanceStaging(this, task, stagingPath, instName, instGroup); +} + +#include "InstanceCopyTask.h" +Task * FolderInstanceProvider::copyTask(const InstancePtr& oldInstance, const QString& instName, const QString& instGroup, const QString& instIcon, bool copySaves) +{ + auto stagingPath = getStagedInstancePath(); + auto task = new InstanceCopyTask(m_globalSettings, stagingPath, oldInstance, instName, instIcon, instGroup, copySaves); + return new FolderInstanceStaging(this, task, stagingPath, instName, instGroup); +} QString FolderInstanceProvider::getStagedInstancePath() { @@ -395,3 +468,4 @@ bool FolderInstanceProvider::destroyStagingPath(const QString& keyPath) return FS::deletePath(keyPath); } +#include "FolderInstanceProvider.moc" diff --git a/api/logic/InstanceCopyTask.cpp b/api/logic/InstanceCopyTask.cpp index e9ed9a82..65c6b9ff 100644 --- a/api/logic/InstanceCopyTask.cpp +++ b/api/logic/InstanceCopyTask.cpp @@ -6,10 +6,10 @@ #include "pathmatcher/RegexpMatcher.h" #include -InstanceCopyTask::InstanceCopyTask(SettingsObjectPtr settings, BaseInstanceProvider* target, InstancePtr origInstance, const QString& instName, const QString& instIcon, const QString& instGroup, bool copySaves) +InstanceCopyTask::InstanceCopyTask(SettingsObjectPtr settings, const QString & stagingPath, InstancePtr origInstance, const QString& instName, const QString& instIcon, const QString& instGroup, bool copySaves) { m_globalSettings = settings; - m_target = target; + m_stagingPath = stagingPath; m_origInstance = origInstance; m_instName = instName; m_instIcon = instIcon; @@ -27,7 +27,7 @@ InstanceCopyTask::InstanceCopyTask(SettingsObjectPtr settings, BaseInstanceProvi void InstanceCopyTask::executeTask() { setStatus(tr("Copying instance %1").arg(m_origInstance->name())); - m_stagingPath = m_target->getStagedInstancePath(); + FS::copy folderCopy(m_origInstance->instanceRoot(), m_stagingPath); folderCopy.followSymlinks(false).blacklist(m_matcher.get()); @@ -42,7 +42,6 @@ void InstanceCopyTask::copyFinished() auto successful = m_copyFuture.result(); if(!successful) { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Instance folder copy failed.")); return; } @@ -56,13 +55,11 @@ void InstanceCopyTask::copyFinished() InstancePtr inst(new NullInstance(m_globalSettings, instanceSettings, m_stagingPath)); inst->setName(m_instName); inst->setIconKey(m_instIcon); - m_target->commitStagedInstance(m_stagingPath, m_instName, m_instGroup); emitSucceeded(); } void InstanceCopyTask::copyAborted() { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Instance folder copy has been aborted.")); return; } diff --git a/api/logic/InstanceCopyTask.h b/api/logic/InstanceCopyTask.h index 28fd3f40..dc46bfec 100644 --- a/api/logic/InstanceCopyTask.h +++ b/api/logic/InstanceCopyTask.h @@ -17,7 +17,7 @@ class MULTIMC_LOGIC_EXPORT InstanceCopyTask : public Task { Q_OBJECT public: - explicit InstanceCopyTask(SettingsObjectPtr settings, BaseInstanceProvider * target, InstancePtr origInstance, const QString &instName, + explicit InstanceCopyTask(SettingsObjectPtr settings, const QString & stagingPath, InstancePtr origInstance, const QString &instName, const QString &instIcon, const QString &instGroup, bool copySaves); protected: @@ -28,7 +28,6 @@ protected: private: /* data */ SettingsObjectPtr m_globalSettings; - BaseInstanceProvider * m_target = nullptr; InstancePtr m_origInstance; QString m_instName; QString m_instIcon; diff --git a/api/logic/InstanceCreationTask.cpp b/api/logic/InstanceCreationTask.cpp index a615d6ff..a7ea1e66 100644 --- a/api/logic/InstanceCreationTask.cpp +++ b/api/logic/InstanceCreationTask.cpp @@ -6,11 +6,11 @@ //FIXME: remove this #include "minecraft/onesix/OneSixInstance.h" -InstanceCreationTask::InstanceCreationTask(SettingsObjectPtr settings, BaseInstanceProvider* target, BaseVersionPtr version, +InstanceCreationTask::InstanceCreationTask(SettingsObjectPtr settings, const QString & stagingPath, BaseVersionPtr version, const QString& instName, const QString& instIcon, const QString& instGroup) { m_globalSettings = settings; - m_target = target; + m_stagingPath = stagingPath; m_instName = instName; m_instIcon = instIcon; m_instGroup = instGroup; @@ -20,19 +20,17 @@ InstanceCreationTask::InstanceCreationTask(SettingsObjectPtr settings, BaseInsta void InstanceCreationTask::executeTask() { setStatus(tr("Creating instance from version %1").arg(m_version->name())); - - QString stagingPath = m_target->getStagedInstancePath(); - QDir rootDir(stagingPath); - - auto instanceSettings = std::make_shared(FS::PathCombine(stagingPath, "instance.cfg")); - instanceSettings->registerSetting("InstanceType", "Legacy"); - - instanceSettings->set("InstanceType", "OneSix"); - InstancePtr inst(new OneSixInstance(m_globalSettings, instanceSettings, stagingPath)); - inst->setIntendedVersionId(m_version->descriptor()); - inst->setName(m_instName); - inst->setIconKey(m_instIcon); - inst->init(); - m_target->commitStagedInstance(stagingPath, m_instName, m_instGroup); + { + auto instanceSettings = std::make_shared(FS::PathCombine(m_stagingPath, "instance.cfg")); + instanceSettings->suspendSave(); + instanceSettings->registerSetting("InstanceType", "Legacy"); + instanceSettings->set("InstanceType", "OneSix"); + OneSixInstance inst(m_globalSettings, instanceSettings, m_stagingPath); + inst.setIntendedVersionId(m_version->descriptor()); + inst.setName(m_instName); + inst.setIconKey(m_instIcon); + inst.init(); + instanceSettings->resumeSave(); + } emitSucceeded(); } diff --git a/api/logic/InstanceCreationTask.h b/api/logic/InstanceCreationTask.h index b4ade320..49fd4615 100644 --- a/api/logic/InstanceCreationTask.h +++ b/api/logic/InstanceCreationTask.h @@ -7,13 +7,11 @@ #include "settings/SettingsObject.h" #include "BaseVersion.h" -class BaseInstanceProvider; - class MULTIMC_LOGIC_EXPORT InstanceCreationTask : public Task { Q_OBJECT public: - explicit InstanceCreationTask(SettingsObjectPtr settings, BaseInstanceProvider * target, BaseVersionPtr version, const QString &instName, + explicit InstanceCreationTask(SettingsObjectPtr settings, const QString & stagingPath, BaseVersionPtr version, const QString &instName, const QString &instIcon, const QString &instGroup); protected: @@ -22,7 +20,7 @@ protected: private: /* data */ SettingsObjectPtr m_globalSettings; - BaseInstanceProvider * m_target; + QString m_stagingPath; BaseVersionPtr m_version; QString m_instName; QString m_instIcon; diff --git a/api/logic/InstanceImportTask.cpp b/api/logic/InstanceImportTask.cpp index a796815b..5a824cb1 100644 --- a/api/logic/InstanceImportTask.cpp +++ b/api/logic/InstanceImportTask.cpp @@ -16,12 +16,12 @@ #include "minecraft/flame/PackManifest.h" #include "Json.h" -InstanceImportTask::InstanceImportTask(SettingsObjectPtr settings, const QUrl sourceUrl, BaseInstanceProvider * target, +InstanceImportTask::InstanceImportTask(SettingsObjectPtr settings, const QUrl sourceUrl, const QString & stagingPath, const QString &instName, const QString &instIcon, const QString &instGroup) { m_globalSettings = settings; m_sourceUrl = sourceUrl; - m_target = target; + m_stagingPath = stagingPath; m_instName = instName; m_instIcon = instIcon; m_instGroup = instGroup; @@ -72,30 +72,9 @@ void InstanceImportTask::downloadProgressChanged(qint64 current, qint64 total) setProgress(current / 2, total); } -static QFileInfo findRecursive(const QString &dir, const QString &name) -{ - for (const auto info : QDir(dir).entryInfoList(QDir::NoDotAndDotDot | QDir::Dirs | QDir::Files, QDir::DirsLast)) - { - if (info.isFile() && info.fileName() == name) - { - return info; - } - else if (info.isDir()) - { - const QFileInfo res = findRecursive(info.absoluteFilePath(), name); - if (res.isFile() && res.exists()) - { - return res; - } - } - } - return QFileInfo(); -} - void InstanceImportTask::processZipPack() { setStatus(tr("Extracting modpack")); - m_stagingPath = m_target->getStagedInstancePath(); QDir extractDir(m_stagingPath); qDebug() << "Attempting to create instance from" << m_archivePath; @@ -143,7 +122,6 @@ void InstanceImportTask::extractFinished() m_packZip.reset(); if (m_extractFuture.result().isEmpty()) { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Failed to extract modpack")); return; } @@ -189,7 +167,6 @@ void InstanceImportTask::extractFinished() processMultiMC(); return; case ModpackType::Unknown: - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Archive does not contain a recognized modpack type.")); return; } @@ -197,7 +174,6 @@ void InstanceImportTask::extractFinished() void InstanceImportTask::extractAborted() { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Instance import has been aborted.")); return; } @@ -218,7 +194,6 @@ void InstanceImportTask::processFlame() } catch (JSONValidationError & e) { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Could not understand pack manifest:\n") + e.cause()); return; } @@ -228,7 +203,6 @@ void InstanceImportTask::processFlame() QString mcPath = FS::PathCombine(m_stagingPath, "minecraft"); if (!QFile::rename(overridePath, mcPath)) { - m_target->destroyStagingPath(m_stagingPath); emitFailed(tr("Could not rename the overrides folder:\n") + pack.overrides); return; } @@ -331,18 +305,11 @@ void InstanceImportTask::processFlame() connect(m_filesNetJob.get(), &NetJob::succeeded, this, [&]() { m_filesNetJob.reset(); - if (!m_target->commitStagedInstance(m_stagingPath, m_instName, m_instGroup)) - { - m_target->destroyStagingPath(m_stagingPath); - emitFailed(tr("Unable to commit instance")); - return; - } emitSucceeded(); } ); connect(m_filesNetJob.get(), &NetJob::failed, [&](QString reason) { - m_target->destroyStagingPath(m_stagingPath); m_filesNetJob.reset(); emitFailed(reason); }); @@ -356,7 +323,6 @@ void InstanceImportTask::processFlame() ); connect(m_modIdResolver.get(), &Flame::FileResolvingTask::failed, [&](QString reason) { - m_target->destroyStagingPath(m_stagingPath); m_modIdResolver.reset(); emitFailed(tr("Unable to resolve mod IDs:\n") + reason); }); @@ -406,11 +372,5 @@ void InstanceImportTask::processMultiMC() iconList->installIcons({importIconPath}); } } - if (!m_target->commitStagedInstance(m_stagingPath, m_instName, m_instGroup)) - { - m_target->destroyStagingPath(m_stagingPath); - emitFailed(tr("Unable to commit instance")); - return; - } emitSucceeded(); } diff --git a/api/logic/InstanceImportTask.h b/api/logic/InstanceImportTask.h index e5f35b3a..99397009 100644 --- a/api/logic/InstanceImportTask.h +++ b/api/logic/InstanceImportTask.h @@ -20,7 +20,7 @@ class MULTIMC_LOGIC_EXPORT InstanceImportTask : public Task { Q_OBJECT public: - explicit InstanceImportTask(SettingsObjectPtr settings, const QUrl sourceUrl, BaseInstanceProvider * target, const QString &instName, + explicit InstanceImportTask(SettingsObjectPtr settings, const QUrl sourceUrl, const QString & stagingPath, const QString &instName, const QString &instIcon, const QString &instGroup); protected: @@ -44,7 +44,6 @@ private: /* data */ NetJobPtr m_filesNetJob; shared_qobject_ptr m_modIdResolver; QUrl m_sourceUrl; - BaseInstanceProvider * m_target; QString m_archivePath; bool m_downloadRequired = false; QString m_instName;