From 51a71d04715bfc47c1688bd7606b3c4f043cd3d8 Mon Sep 17 00:00:00 2001 From: Trial97 Date: Mon, 21 Oct 2024 23:53:23 +0300 Subject: [PATCH 1/3] skip QSaveFile temprary files Signed-off-by: Trial97 (cherry picked from commit 562c3013269dbb9cad411f58ded333dee1aea158) --- launcher/Application.cpp | 23 ++++++ launcher/Application.h | 11 +++ launcher/CMakeLists.txt | 1 + launcher/FileSystem.cpp | 22 +++--- launcher/PSaveFile.h | 70 +++++++++++++++++++ launcher/minecraft/World.cpp | 4 +- .../minecraft/mod/tasks/BasicFolderLoadTask.h | 4 ++ .../minecraft/mod/tasks/ModFolderLoadTask.cpp | 4 ++ launcher/modplatform/packwiz/Packwiz.cpp | 4 +- launcher/net/FileSink.cpp | 2 +- launcher/net/FileSink.h | 5 +- launcher/settings/INIFile.cpp | 1 - launcher/ui/dialogs/ExportInstanceDialog.cpp | 1 - 13 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 launcher/PSaveFile.h diff --git a/launcher/Application.cpp b/launcher/Application.cpp index cfae3a807..686d934c2 100644 --- a/launcher/Application.cpp +++ b/launcher/Application.cpp @@ -1883,3 +1883,26 @@ const QString Application::javaPath() { return m_settings->get("JavaDir").toString(); } + +void Application::addQSavePath(QString path) +{ + QMutexLocker locker(&m_qsaveResourcesMutex); + m_qsaveResources.insert(path); +} + +void Application::removeQSavePath(QString path) +{ + QMutexLocker locker(&m_qsaveResourcesMutex); + m_qsaveResources.remove(path); +} + +bool Application::checkQSavePath(QString path) +{ + QMutexLocker locker(&m_qsaveResourcesMutex); + for (auto r : m_qsaveResources) { + if (path.contains(r)) { + return true; + } + } + return false; +} \ No newline at end of file diff --git a/launcher/Application.h b/launcher/Application.h index bd1cb2dea..363130cdd 100644 --- a/launcher/Application.h +++ b/launcher/Application.h @@ -42,6 +42,8 @@ #include #include #include +#include +#include #include #include @@ -303,4 +305,13 @@ class Application : public QApplication { QList m_urlsToImport; QString m_instanceIdToShowWindowOf; std::unique_ptr logFile; + + public: + void addQSavePath(QString); + void removeQSavePath(QString); + bool checkQSavePath(QString); + + private: + QSet m_qsaveResources; + mutable QMutex m_qsaveResourcesMutex; }; diff --git a/launcher/CMakeLists.txt b/launcher/CMakeLists.txt index a70fe668a..0f0949a3a 100644 --- a/launcher/CMakeLists.txt +++ b/launcher/CMakeLists.txt @@ -30,6 +30,7 @@ set(CORE_SOURCES StringUtils.cpp QVariantUtils.h RuntimeContext.h + PSaveFile.h # Basic instance manipulation tasks (derived from InstanceTask) InstanceCreationTask.h diff --git a/launcher/FileSystem.cpp b/launcher/FileSystem.cpp index 7f38cff04..512de28c2 100644 --- a/launcher/FileSystem.cpp +++ b/launcher/FileSystem.cpp @@ -45,7 +45,6 @@ #include #include #include -#include #include #include #include @@ -54,6 +53,7 @@ #include #include "DesktopServices.h" +#include "PSaveFile.h" #include "StringUtils.h" #if defined Q_OS_WIN32 @@ -191,8 +191,8 @@ void ensureExists(const QDir& dir) void write(const QString& filename, const QByteArray& data) { ensureExists(QFileInfo(filename).dir()); - QSaveFile file(filename); - if (!file.open(QSaveFile::WriteOnly)) { + PSaveFile file(filename); + if (!file.open(PSaveFile::WriteOnly)) { throw FileSystemException("Couldn't open " + filename + " for writing: " + file.errorString()); } if (data.size() != file.write(data)) { @@ -213,8 +213,8 @@ void appendSafe(const QString& filename, const QByteArray& data) buffer = QByteArray(); } buffer.append(data); - QSaveFile file(filename); - if (!file.open(QSaveFile::WriteOnly)) { + PSaveFile file(filename); + if (!file.open(PSaveFile::WriteOnly)) { throw FileSystemException("Couldn't open " + filename + " for writing: " + file.errorString()); } if (buffer.size() != file.write(buffer)) { @@ -971,8 +971,7 @@ bool createShortcut(QString destination, QString target, QStringList args, QStri if (!args.empty()) argstring = " \"" + args.join("\" \"") + "\""; - stream << "#!/bin/bash" - << "\n"; + stream << "#!/bin/bash" << "\n"; stream << "\"" << target << "\" " << argstring << "\n"; stream.flush(); @@ -1016,12 +1015,9 @@ bool createShortcut(QString destination, QString target, QStringList args, QStri if (!args.empty()) argstring = " '" + args.join("' '") + "'"; - stream << "[Desktop Entry]" - << "\n"; - stream << "Type=Application" - << "\n"; - stream << "Categories=Game;ActionGame;AdventureGame;Simulation" - << "\n"; + stream << "[Desktop Entry]" << "\n"; + stream << "Type=Application" << "\n"; + stream << "Categories=Game;ActionGame;AdventureGame;Simulation" << "\n"; stream << "Exec=\"" << target.toLocal8Bit() << "\"" << argstring.toLocal8Bit() << "\n"; stream << "Name=" << name.toLocal8Bit() << "\n"; if (!icon.isEmpty()) { diff --git a/launcher/PSaveFile.h b/launcher/PSaveFile.h new file mode 100644 index 000000000..e0b5a7a2c --- /dev/null +++ b/launcher/PSaveFile.h @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-3.0-only +/* + * Prism Launcher - Minecraft Launcher + * Copyright (c) 2023-2024 Trial97 + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +#pragma once + +#include +#include "Application.h" + +#if defined(LAUNCHER_APPLICATION) + +/* PSaveFile + * A class that mimics QSaveFile for Windows. + * + * When reading resources, we need to avoid accessing temporary files + * generated by QSaveFile. If we start reading such a file, we may + * inadvertently keep it open while QSaveFile is trying to remove it, + * or we might detect the file just before it is removed, leading to + * race conditions and errors. + * + * Unfortunately, QSaveFile doesn't provide a way to retrieve the + * temporary file name or to set a specific template for the temporary + * file name it uses. By default, QSaveFile appends a `.XXXXXX` suffix + * to the original file name, where the `XXXXXX` part is dynamically + * generated to ensure uniqueness. + * + * This class acts like a lock by adding and removing the target file + * name into/from a global string set, helping to manage access to + * files during critical operations. + * + * Note: Please do not use the `setFileName` function directly, as it + * is not virtual and cannot be overridden. + */ +class PSaveFile : public QSaveFile { + public: + PSaveFile(const QString& name) : QSaveFile(name) + { + if (auto app = APPLICATION_DYN) { + app->addQSavePath(name + "."); + } + } + PSaveFile(const QString& name, QObject* parent) : QSaveFile(name, parent) + { + if (auto app = APPLICATION_DYN) { + app->addQSavePath(name + "."); + } + } + virtual ~PSaveFile() + { + if (auto app = APPLICATION_DYN) { + app->removeQSavePath(fileName() + "."); + } + } +}; +#else +#define PSaveFile QSaveFile +#endif \ No newline at end of file diff --git a/launcher/minecraft/World.cpp b/launcher/minecraft/World.cpp index 1eba148a5..bd28f9e9a 100644 --- a/launcher/minecraft/World.cpp +++ b/launcher/minecraft/World.cpp @@ -38,7 +38,6 @@ #include #include #include -#include #include #include @@ -57,6 +56,7 @@ #include #include "FileSystem.h" +#include "PSaveFile.h" using std::nullopt; using std::optional; @@ -183,7 +183,7 @@ bool putLevelDatDataToFS(const QFileInfo& file, QByteArray& data) if (fullFilePath.isNull()) { return false; } - QSaveFile f(fullFilePath); + PSaveFile f(fullFilePath); if (!f.open(QIODevice::WriteOnly)) { return false; } diff --git a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h index 2bce2c137..fb2e22de6 100644 --- a/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h +++ b/launcher/minecraft/mod/tasks/BasicFolderLoadTask.h @@ -7,6 +7,7 @@ #include +#include "Application.h" #include "FileSystem.h" #include "minecraft/mod/Resource.h" @@ -52,6 +53,9 @@ class BasicFolderLoadTask : public Task { m_dir.refresh(); for (auto entry : m_dir.entryInfoList()) { auto filePath = entry.absoluteFilePath(); + if (auto app = APPLICATION_DYN; app && app->checkQSavePath(filePath)) { + continue; + } auto newFilePath = FS::getUniqueResourceName(filePath); if (newFilePath != filePath) { FS::move(filePath, newFilePath); diff --git a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp index 501d5be13..63996e584 100644 --- a/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp +++ b/launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp @@ -36,6 +36,7 @@ #include "ModFolderLoadTask.h" +#include "Application.h" #include "FileSystem.h" #include "minecraft/mod/MetadataHandler.h" @@ -65,6 +66,9 @@ void ModFolderLoadTask::executeTask() m_mods_dir.refresh(); for (auto entry : m_mods_dir.entryInfoList()) { auto filePath = entry.absoluteFilePath(); + if (auto app = APPLICATION_DYN; app && app->checkQSavePath(filePath)) { + continue; + } auto newFilePath = FS::getUniqueResourceName(filePath); if (newFilePath != filePath) { FS::move(filePath, newFilePath); diff --git a/launcher/modplatform/packwiz/Packwiz.cpp b/launcher/modplatform/packwiz/Packwiz.cpp index 325b0a6e4..272900f0e 100644 --- a/launcher/modplatform/packwiz/Packwiz.cpp +++ b/launcher/modplatform/packwiz/Packwiz.cpp @@ -72,7 +72,7 @@ auto stringEntry(toml::table table, QString entry_name) -> QString { auto node = table[StringUtils::toStdString(entry_name)]; if (!node) { - qCritical() << "Failed to read str property '" + entry_name + "' in mod metadata."; + qWarning() << "Failed to read str property '" + entry_name + "' in mod metadata."; return {}; } @@ -83,7 +83,7 @@ auto intEntry(toml::table table, QString entry_name) -> int { auto node = table[StringUtils::toStdString(entry_name)]; if (!node) { - qCritical() << "Failed to read int property '" + entry_name + "' in mod metadata."; + qWarning() << "Failed to read int property '" + entry_name + "' in mod metadata."; return {}; } diff --git a/launcher/net/FileSink.cpp b/launcher/net/FileSink.cpp index 1ecb21fdf..95c1a8f44 100644 --- a/launcher/net/FileSink.cpp +++ b/launcher/net/FileSink.cpp @@ -55,7 +55,7 @@ Task::State FileSink::init(QNetworkRequest& request) } wroteAnyData = false; - m_output_file.reset(new QSaveFile(m_filename)); + m_output_file.reset(new PSaveFile(m_filename)); if (!m_output_file->open(QIODevice::WriteOnly)) { qCCritical(taskNetLogC) << "Could not open " + m_filename + " for writing"; return Task::State::Failed; diff --git a/launcher/net/FileSink.h b/launcher/net/FileSink.h index 816254ff9..272f8ddc3 100644 --- a/launcher/net/FileSink.h +++ b/launcher/net/FileSink.h @@ -35,8 +35,7 @@ #pragma once -#include - +#include "PSaveFile.h" #include "Sink.h" namespace Net { @@ -60,6 +59,6 @@ class FileSink : public Sink { protected: QString m_filename; bool wroteAnyData = false; - std::unique_ptr m_output_file; + std::unique_ptr m_output_file; }; } // namespace Net diff --git a/launcher/settings/INIFile.cpp b/launcher/settings/INIFile.cpp index e97741f20..2c7620e65 100644 --- a/launcher/settings/INIFile.cpp +++ b/launcher/settings/INIFile.cpp @@ -39,7 +39,6 @@ #include #include -#include #include #include #include diff --git a/launcher/ui/dialogs/ExportInstanceDialog.cpp b/launcher/ui/dialogs/ExportInstanceDialog.cpp index 9f2b3ac42..7e00f18f4 100644 --- a/launcher/ui/dialogs/ExportInstanceDialog.cpp +++ b/launcher/ui/dialogs/ExportInstanceDialog.cpp @@ -51,7 +51,6 @@ #include #include #include -#include #include #include #include From 85422427b9794a3bb6765e9947c03844862035e4 Mon Sep 17 00:00:00 2001 From: Trial97 Date: Tue, 22 Oct 2024 09:41:00 +0300 Subject: [PATCH 2/3] Replaced QSet with QHash Signed-off-by: Trial97 (cherry picked from commit 73d33f93b30f658f9671358ac52bf4e03afeaefd) --- launcher/Application.cpp | 13 +++++++++---- launcher/Application.h | 3 +-- launcher/PSaveFile.h | 27 ++++++++++++++------------- 3 files changed, 24 insertions(+), 19 deletions(-) diff --git a/launcher/Application.cpp b/launcher/Application.cpp index 686d934c2..ea749ca4c 100644 --- a/launcher/Application.cpp +++ b/launcher/Application.cpp @@ -1887,20 +1887,25 @@ const QString Application::javaPath() void Application::addQSavePath(QString path) { QMutexLocker locker(&m_qsaveResourcesMutex); - m_qsaveResources.insert(path); + m_qsaveResources[path] = m_qsaveResources.value(path, 0) + 1; } void Application::removeQSavePath(QString path) { QMutexLocker locker(&m_qsaveResourcesMutex); - m_qsaveResources.remove(path); + auto count = m_qsaveResources.value(path, 0) - 1; + if (count <= 0) { + m_qsaveResources.remove(path); + } else { + m_qsaveResources[path] = count; + } } bool Application::checkQSavePath(QString path) { QMutexLocker locker(&m_qsaveResourcesMutex); - for (auto r : m_qsaveResources) { - if (path.contains(r)) { + for (auto partialPath : m_qsaveResources.keys()) { + if (path.startsWith(partialPath) && m_qsaveResources.value(partialPath, 0) > 0) { return true; } } diff --git a/launcher/Application.h b/launcher/Application.h index 363130cdd..692625f4a 100644 --- a/launcher/Application.h +++ b/launcher/Application.h @@ -43,7 +43,6 @@ #include #include #include -#include #include #include @@ -312,6 +311,6 @@ class Application : public QApplication { bool checkQSavePath(QString); private: - QSet m_qsaveResources; + QHash m_qsaveResources; mutable QMutex m_qsaveResourcesMutex; }; diff --git a/launcher/PSaveFile.h b/launcher/PSaveFile.h index e0b5a7a2c..ba6154ad8 100644 --- a/launcher/PSaveFile.h +++ b/launcher/PSaveFile.h @@ -17,6 +17,7 @@ */ #pragma once +#include #include #include "Application.h" @@ -46,24 +47,24 @@ */ class PSaveFile : public QSaveFile { public: - PSaveFile(const QString& name) : QSaveFile(name) - { - if (auto app = APPLICATION_DYN) { - app->addQSavePath(name + "."); - } - } - PSaveFile(const QString& name, QObject* parent) : QSaveFile(name, parent) - { - if (auto app = APPLICATION_DYN) { - app->addQSavePath(name + "."); - } - } + PSaveFile(const QString& name) : QSaveFile(name) { addPath(name); } + PSaveFile(const QString& name, QObject* parent) : QSaveFile(name, parent) { addPath(name); } virtual ~PSaveFile() { if (auto app = APPLICATION_DYN) { - app->removeQSavePath(fileName() + "."); + app->removeQSavePath(m_absoluteFilePath); } } + + private: + void addPath(const QString& path) + { + m_absoluteFilePath = QFileInfo(path).absoluteFilePath() + "."; // add dot for tmp files only + if (auto app = APPLICATION_DYN) { + app->addQSavePath(m_absoluteFilePath); + } + } + QString m_absoluteFilePath; }; #else #define PSaveFile QSaveFile From b983ae0fb000b0a413115f4b8bd64f8447cca80b Mon Sep 17 00:00:00 2001 From: Trial97 Date: Tue, 22 Oct 2024 13:43:16 +0300 Subject: [PATCH 3/3] fix small leak Signed-off-by: Trial97 (cherry picked from commit 836aebc0d91fe8658052b8c4a5a4ab31da4a4b15) --- launcher/ui/pages/instance/ModFolderPage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/launcher/ui/pages/instance/ModFolderPage.cpp b/launcher/ui/pages/instance/ModFolderPage.cpp index f2feb8c7f..a6fbf5088 100644 --- a/launcher/ui/pages/instance/ModFolderPage.cpp +++ b/launcher/ui/pages/instance/ModFolderPage.cpp @@ -124,7 +124,7 @@ ModFolderPage::ModFolderPage(BaseInstance* inst, std::shared_ptr ui->actionsToolbar->addAction(ui->actionVisitItemPage); connect(ui->actionVisitItemPage, &QAction::triggered, this, &ModFolderPage::visitModPages); - auto changeVersion = new QAction(tr("Change Version")); + auto changeVersion = new QAction(tr("Change Version"), this); changeVersion->setToolTip(tr("Change mod version")); changeVersion->setEnabled(false); ui->actionsToolbar->insertActionAfter(ui->actionUpdateItem, changeVersion);