From ababf3d70b68c9d76673dedcc846a6f751c82b96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Mon, 7 Sep 2026 03:03:57 +0200 Subject: [PATCH] [Oracle/Client] Harden oracle progress workers and quit prompt Address review feedback on the download-progress change: decompress and read sets files off the UI thread, cancel the load/import workers before the wizard can tear down the importer, and show an 'Extracting file...' status plus a clean 100% tail so the poll never looks stuck. Quitting Cockatrice while a card database update runs now asks for confirmation. --- cockatrice/src/interface/window_main.cpp | 11 + oracle/src/oracleimporter.cpp | 8 + oracle/src/oracleimporter.h | 19 ++ oracle/src/oraclewizard.cpp | 11 + oracle/src/oraclewizard.h | 1 + oracle/src/pages.cpp | 351 +++++++++++++++-------- oracle/src/pages.h | 24 +- oracle/src/pagetemplates.h | 8 + 8 files changed, 304 insertions(+), 129 deletions(-) diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index 5afa5fbf0..43f1de9dd 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -844,6 +844,17 @@ void MainWindow::closeEvent(QCloseEvent *event) } bClosingDown = true; + if (cardUpdateProcess && cardUpdateProcess->state() != QProcess::NotRunning) { + if (QMessageBox::question(this, tr("Are you sure?"), + tr("A card database update is still running. Quitting now will cancel it.\n" + "Are you sure you want to quit?"), + QMessageBox::Yes | QMessageBox::No, QMessageBox::No) == QMessageBox::No) { + event->ignore(); + bClosingDown = false; + return; + } + } + if (!tabSupervisor->close()) { event->ignore(); bClosingDown = false; diff --git a/oracle/src/oracleimporter.cpp b/oracle/src/oracleimporter.cpp index 6a83fd96c..a28667c50 100644 --- a/oracle/src/oracleimporter.cpp +++ b/oracle/src/oracleimporter.cpp @@ -557,6 +557,8 @@ int OracleImporter::startImport() { static ICardSetPriorityController *noOpController = new NoopCardSetPriorityController(); + importCancelled.storeRelease(0); + // Pre-allocate the cards hash to avoid rehashing during import. Keys are // distinct card names while raw ranges only count printings (AllPrintings // ~100k printings vs ~35k names), so this over-reserves somewhat; an exact @@ -576,6 +578,12 @@ int OracleImporter::startImport() int setIndex = 0; for (const SetToDownload &curSetToParse : allSets) { + if (importCancelled.loadAcquire()) { + // The wizard was closed mid-import: stop at the next set boundary so + // the caller can wait for this future without processing every set. + break; + } + CardSetPtr newSet = CardSet::newInstance(noOpController, curSetToParse.getShortName(), curSetToParse.getLongName(), curSetToParse.getSetType(), curSetToParse.getReleaseDate(), curSetToParse.getPriority()); diff --git a/oracle/src/oracleimporter.h b/oracle/src/oracleimporter.h index 62927fefb..748056772 100644 --- a/oracle/src/oracleimporter.h +++ b/oracle/src/oracleimporter.h @@ -3,6 +3,7 @@ #include "raw_json_scanner.h" +#include #include #include #include @@ -163,6 +164,13 @@ private: */ bool progressReporting = true; + /** + * Atomic "please stop importing" flag. startImport() checks it between sets + * so a wizard being closed mid-import can be torn down without waiting for + * the whole import (or racing it). + */ + QAtomicInt importCancelled; + CardInfoPtr addCard(QString name, const QString &text, bool isToken, @@ -193,6 +201,17 @@ public: */ bool readSetsFromByteArray(QByteArray data); int startImport(); + /** + * @brief Requests an in-flight startImport() to stop at the next set boundary. + * + * Works by setting an atomic flag that startImport() polls between sets, so + * cancelImport() followed by a short waitForFinished() on the running future is + * safe the moment the wizard is about to be destroyed. + */ + void cancelImport() + { + importCancelled.storeRelease(1); + } bool saveToFile(const QString &fileName, const QString &sourceUrl, const QString &sourceVersion); int importCardsFromSet(const CardSetPtr ¤tSet, const QJsonArray &cardsList); /** diff --git a/oracle/src/oraclewizard.cpp b/oracle/src/oraclewizard.cpp index ec9cb41bc..dd3ac6459 100644 --- a/oracle/src/oraclewizard.cpp +++ b/oracle/src/oraclewizard.cpp @@ -110,6 +110,17 @@ void OracleWizard::accept() QDialog::accept(); } +void OracleWizard::reject() +{ + // The wizard is being closed while a page may still run a worker on the + // importer. Ask it to stop before the wizard (and the importer child) is + // destroyed, so the worker thread never touches freed memory. + if (auto *active = dynamic_cast(currentPage())) { + active->cancelWork(); + } + QWizard::reject(); +} + void OracleWizard::runInBackground() { backgroundMode = true; diff --git a/oracle/src/oraclewizard.h b/oracle/src/oraclewizard.h index e6c09fedf..9a509ce5e 100644 --- a/oracle/src/oraclewizard.h +++ b/oracle/src/oraclewizard.h @@ -23,6 +23,7 @@ class OracleWizard : public QWizard public: explicit OracleWizard(QWidget *parent = nullptr); void accept() override; + void reject() override; void enableButtons(); void disableButtons(); void retranslateUi(); diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index 263712e5f..002cf9993 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -71,6 +71,101 @@ static void emitBackgroundProgress(const char *stage, qint64 done, qint64 total) out.flush(); } +namespace +{ + +/** + * @brief Decompresses and dispatches a sets-file payload on a worker thread. + * + * Iteratively unwraps xz/zip compression, then either hands the JSON to the + * importer (which reports scan progress via dataReadProgress) or returns the raw + * XML for the plain-XML path. Must never touch the wizard or the page: the caller + * consumes the returned LoadSetsResult on the UI thread in importFinished(). + */ +LoadSetsResult loadSetsData(const QPointer &importer, QByteArray data) +{ + LoadSetsResult result; + + while (true) { + if (data.startsWith(XZ_SIGNATURE)) { +#ifdef HAS_LZMA + QBuffer inBuffer(&data); + QByteArray decompressed; + QBuffer outBuffer(&decompressed); + inBuffer.open(QBuffer::ReadOnly); + outBuffer.open(QBuffer::WriteOnly); + XzDecompressor xz; + if (!xz.decompress(&inBuffer, &outBuffer)) { + result.errorMessage = LoadSetsPage::tr("Xz extraction failed."); + result.offerUncompressedFallback = true; + return result; + } + data = decompressed; + continue; +#else + result.errorMessage = + LoadSetsPage::tr("Sorry, this version of Oracle does not support xz compressed files."); + result.offerUncompressedFallback = true; + return result; +#endif + } + + if (data.startsWith(ZIP_SIGNATURE)) { +#ifdef HAS_ZLIB + QBuffer inBuffer(&data); + UnZip uz; + const UnZip::ErrorCode openEc = uz.openArchive(&inBuffer); + if (openEc != UnZip::Ok) { + result.errorMessage = LoadSetsPage::tr("Failed to open Zip archive: %1.").arg(uz.formatError(openEc)); + result.offerUncompressedFallback = true; + return result; + } + if (uz.fileList().size() != 1) { + result.errorMessage = + LoadSetsPage::tr("Zip extraction failed: the Zip archive doesn't contain exactly one file."); + result.offerUncompressedFallback = true; + return result; + } + const QString fileName = uz.fileList().at(0); + QByteArray decompressed; + QBuffer outBuffer(&decompressed); + outBuffer.open(QBuffer::ReadWrite); + const UnZip::ErrorCode ec = uz.extractFile(fileName, &outBuffer); + uz.closeArchive(); + if (ec != UnZip::Ok) { + result.errorMessage = LoadSetsPage::tr("Zip extraction failed: %1.").arg(uz.formatError(ec)); + result.offerUncompressedFallback = true; + return result; + } + data = decompressed; + continue; +#else + result.errorMessage = LoadSetsPage::tr("Sorry, this version of Oracle does not support zipped files."); + result.offerUncompressedFallback = true; + return result; +#endif + } + break; + } + + if (data.startsWith("<")) { + result.ok = true; + result.plainXml = true; + result.xmlData = std::move(data); + return result; + } + + if (data.startsWith("{")) { + result.ok = importer && importer->readSetsFromByteArray(std::move(data)); + return result; + } + + result.errorMessage = LoadSetsPage::tr("Failed to interpret downloaded data."); + return result; +} + +} // namespace + #define TOKENS_URL "https://raw.githubusercontent.com/Cockatrice/Magic-Token/master/tokens.xml" #define SPOILERS_URL "https://raw.githubusercontent.com/Cockatrice/Magic-Spoiler/files/spoiler.xml" @@ -297,18 +392,13 @@ bool LoadSetsPage::validatePage() return false; } - if (!setsFile.open(QIODevice::ReadOnly)) { - QMessageBox::critical(nullptr, tr("Error"), tr("Cannot open file '%1'.").arg(fileLineEdit->text())); - return false; - } - wizard()->disableButtons(); setEnabled(false); wizard()->setCardSourceUrl(setsFile.fileName()); wizard()->setCardSourceVersion("unknown"); - readSetsFromByteArray(setsFile.readAll()); + readSetsFromFile(setsFile.fileName()); } return false; @@ -410,6 +500,7 @@ void LoadSetsPage::updateParsingProgress(int bytesRead, int totalBytes) if (totalBytes <= 0) { return; } + progressBar->setRange(0, totalBytes); progressBar->setValue(bytesRead); const int percent = static_cast((100.0 * bytesRead) / totalBytes); progressLabel->setText(tr("Parsing file (%1%)").arg(percent)); @@ -420,120 +511,76 @@ void LoadSetsPage::scanProgressToStdout(int bytesRead, int totalBytes) emitBackgroundProgress("scan", bytesRead, totalBytes); } -void LoadSetsPage::readSetsFromByteArray(QByteArray _data) +void LoadSetsPage::beginLoadSets(bool compressedFile) { - // show an infinite progressbar + // Show an infinite progressbar while the worker decompresses; the scan + // steals the label via dataReadProgress as soon as it starts. progressBar->setMaximum(0); progressBar->setMinimum(0); progressBar->setValue(0); - progressLabel->setText(tr("Parsing file")); + progressLabel->setText(compressedFile ? tr("Extracting file...") : tr("Parsing file")); progressLabel->show(); progressBar->show(); wizard()->downloadedPlainXml = false; wizard()->xmlData.clear(); - readSetsFromByteArrayRef(_data); + + if (wizard()->backgroundMode) { + connect(wizard()->importer, &OracleImporter::dataReadProgress, this, &LoadSetsPage::scanProgressToStdout, + Qt::UniqueConnection); + } else { + connect(wizard()->importer, &OracleImporter::dataReadProgress, this, &LoadSetsPage::updateParsingProgress, + Qt::UniqueConnection); + } } -void LoadSetsPage::readSetsFromByteArrayRef(QByteArray &_data) +void LoadSetsPage::readSetsFromByteArray(QByteArray _data) { - // unzip the file if needed - if (_data.startsWith(XZ_SIGNATURE)) { -#ifdef HAS_LZMA - // zipped file - auto *inBuffer = new QBuffer(&_data); - auto newData = QByteArray(); - auto *outBuffer = new QBuffer(&newData); - inBuffer->open(QBuffer::ReadOnly); - outBuffer->open(QBuffer::WriteOnly); - XzDecompressor xz; - if (!xz.decompress(inBuffer, outBuffer)) { - zipDownloadFailed(tr("Xz extraction failed.")); - return; + const bool compressed = _data.startsWith(XZ_SIGNATURE) || _data.startsWith(ZIP_SIGNATURE); + beginLoadSets(compressed); + + // Decompress and scan off the UI thread so a large download can't freeze the window. + const QPointer importer = wizard()->importer; + future = QtConcurrent::run( + [importer, data = std::move(_data)]() mutable { return loadSetsData(importer, std::move(data)); }); + watcher.setFuture(future); +} + +void LoadSetsPage::readSetsFromFile(const QString &fileName) +{ + // Peek at the header on the UI thread so the status text can distinguish + // "Extracting file..." from a plain JSON parse; the full read happens in the worker. + QFile headerFile(fileName); + bool compressed = false; + if (headerFile.open(QIODevice::ReadOnly)) { + const QByteArray header = headerFile.read(6); + compressed = header.startsWith(XZ_SIGNATURE) || header.startsWith(ZIP_SIGNATURE); + } + beginLoadSets(compressed); + + // Read, decompress and scan off the UI thread (a plain JSON can be hundreds + // of MB, so even the read itself must not block the window). + const QPointer importer = wizard()->importer; + future = QtConcurrent::run([importer, fileName]() mutable -> LoadSetsResult { + QFile file(fileName); + if (!file.open(QIODevice::ReadOnly)) { + LoadSetsResult readError; + readError.errorMessage = LoadSetsPage::tr("Cannot open file '%1'.").arg(fileName); + return readError; } - _data.clear(); - readSetsFromByteArrayRef(newData); - return; -#else - zipDownloadFailed(tr("Sorry, this version of Oracle does not support xz compressed files.")); + return loadSetsData(importer, file.readAll()); + }); + watcher.setFuture(future); +} - wizard()->enableButtons(); - setEnabled(true); - progressLabel->hide(); - progressBar->hide(); - return; -#endif - } else if (_data.startsWith(ZIP_SIGNATURE)) { -#ifdef HAS_ZLIB - // zipped file - auto *inBuffer = new QBuffer(&_data); - auto newData = QByteArray(); - auto *outBuffer = new QBuffer(&newData); - QString fileName; - UnZip::ErrorCode ec; - UnZip uz; - - ec = uz.openArchive(inBuffer); - if (ec != UnZip::Ok) { - zipDownloadFailed(tr("Failed to open Zip archive: %1.").arg(uz.formatError(ec))); - return; - } - - if (uz.fileList().size() != 1) { - zipDownloadFailed(tr("Zip extraction failed: the Zip archive doesn't contain exactly one file.")); - return; - } - fileName = uz.fileList().at(0); - - outBuffer->open(QBuffer::ReadWrite); - ec = uz.extractFile(fileName, outBuffer); - if (ec != UnZip::Ok) { - zipDownloadFailed(tr("Zip extraction failed: %1.").arg(uz.formatError(ec))); - uz.closeArchive(); - return; - } - _data.clear(); - readSetsFromByteArrayRef(newData); - return; -#else - zipDownloadFailed(tr("Sorry, this version of Oracle does not support zipped files.")); - - wizard()->enableButtons(); - setEnabled(true); - progressLabel->hide(); - progressBar->hide(); - return; -#endif - } else if (_data.startsWith("{")) { - if (wizard()->backgroundMode) { - qInfo() << tr("Parsing file"); - connect(wizard()->importer, &OracleImporter::dataReadProgress, this, &LoadSetsPage::scanProgressToStdout, - Qt::UniqueConnection); - } else { - // Start the computation. - progressBar->setRange(0, static_cast(_data.size())); - progressBar->setValue(0); - progressLabel->setText(tr("Parsing file (0%)")); - connect(wizard()->importer, &OracleImporter::dataReadProgress, this, &LoadSetsPage::updateParsingProgress, - Qt::UniqueConnection); - } - - const QPointer importer = wizard()->importer; - future = QtConcurrent::run([importer, data = std::move(_data)]() mutable { - return importer ? importer->readSetsFromByteArray(std::move(data)) : false; - }); - watcher.setFuture(future); - } else if (_data.startsWith("<")) { - // save xml file and don't do any processing - wizard()->downloadedPlainXml = true; - wizard()->xmlData = std::move(_data); - importFinished(); - } else { - wizard()->enableButtons(); - setEnabled(true); - progressLabel->hide(); - progressBar->hide(); - QMessageBox::critical(this, tr("Error"), tr("Failed to interpret downloaded data.")); +void LoadSetsPage::cancelWork() +{ + // The scan is short-lived; just wait it out before the wizard (and its + // importer) can be torn down underneath the worker thread. + if (future.isRunning()) { + future.cancel(); + watcher.cancel(); + future.waitForFinished(); } } @@ -561,24 +608,52 @@ void LoadSetsPage::importFinished() { wizard()->enableButtons(); setEnabled(true); - progressLabel->hide(); - progressBar->hide(); - const bool hasData = wizard()->downloadedPlainXml || watcher.future().result(); + const LoadSetsResult result = watcher.result(); + + if (result.plainXml) { + wizard()->downloadedPlainXml = true; + wizard()->xmlData = result.xmlData; + } + if (wizard()->backgroundMode) { - if (!hasData) { + progressLabel->hide(); + progressBar->hide(); + if (!result.errorMessage.isEmpty()) { + qWarning() << result.errorMessage; + } else if (!result.ok && !result.plainXml) { qWarning() << tr("The file was retrieved successfully, but it does not contain any sets data."); } emit readyToContinue(); return; } - if (hasData) { - wizard()->next(); - } else { - QMessageBox::critical(this, tr("Error"), - tr("The file was retrieved successfully, but it does not contain any sets data.")); + const auto fail = [this](const QString &message) { + progressLabel->hide(); + progressBar->hide(); + QMessageBox::critical(this, tr("Error"), message); + }; + + if (!result.errorMessage.isEmpty()) { + if (result.offerUncompressedFallback) { + zipDownloadFailed(result.errorMessage); + return; + } + fail(result.errorMessage); + return; } + + if (!result.ok && !result.plainXml) { + fail(tr("The file was retrieved successfully, but it does not contain any sets data.")); + return; + } + + // Snap the bar to 100% so the tail never looks stuck at 99% while the next + // page's own import progress is being set up. + progressBar->setMaximum(1); + progressBar->setValue(1); + progressLabel->setText(tr("Parsing file (100%)")); + wizard()->next(); } SaveSetsPage::SaveSetsPage(QWidget *parent) : OracleWizardPage(parent) @@ -601,15 +676,15 @@ SaveSetsPage::SaveSetsPage(QWidget *parent) : OracleWizardPage(parent) layout->addWidget(pathLabel, 3, 0); layout->addWidget(defaultPathCheckBox, 4, 0); - connect(&importWatcher, &QFutureWatcher::finished, this, &SaveSetsPage::importFinished); - setLayout(layout); } void SaveSetsPage::cleanupPage() { + cancelWork(); + disconnect(wizard()->importer, &OracleImporter::setIndexChanged, this, &SaveSetsPage::updateTotalProgress); + disconnect(&importWatcher, &QFutureWatcher::finished, this, &SaveSetsPage::importFinished); wizard()->importer->clear(); - disconnect(wizard()->importer, &OracleImporter::setIndexChanged, nullptr, nullptr); } void SaveSetsPage::initializePage() @@ -627,21 +702,44 @@ void SaveSetsPage::initializePage() messageLog->clear(); messageLog->show(); progressBar->show(); - progressBar->setRange(0, wizard()->importer->getSets().size()); + + totalSets = wizard()->importer->getSets().size(); + progressBar->setRange(0, totalSets); progressBar->setValue(0); connect(wizard()->importer, &OracleImporter::setIndexChanged, this, &SaveSetsPage::updateTotalProgress, Qt::UniqueConnection); + connect(&importWatcher, &QFutureWatcher::finished, this, &SaveSetsPage::importFinished, Qt::UniqueConnection); wizard()->disableButtons(); + importActive = true; const QPointer importer = wizard()->importer; importFuture = QtConcurrent::run([importer] { return importer ? importer->startImport() : 0; }); importWatcher.setFuture(importFuture); } +void SaveSetsPage::cancelWork() +{ + if (!importActive) { + return; + } + // Ask the worker to stop at the next set boundary, then wait it out so the + // wizard (and the importer it owns) is never torn down under a running thread. + importActive = false; + wizard()->importer->cancelImport(); + importFuture.cancel(); + importWatcher.cancel(); + importFuture.waitForFinished(); +} + void SaveSetsPage::importFinished() { + if (!importActive) { + return; + } + importActive = false; + wizard()->enableButtons(); const int setsImported = importWatcher.result(); @@ -685,20 +783,21 @@ void SaveSetsPage::retranslateUi() void SaveSetsPage::updateTotalProgress(int cardsImported, int setIndex, const QString &setName) { - const bool background = wizard()->backgroundMode; - const int totalSets = wizard()->importer->getSets().size(); + if (!importActive) { + return; + } if (setName.isEmpty()) { progressBar->setValue(progressBar->maximum()); - if (background) { - qInfo() << tr("Import finished: %1 cards.").arg(wizard()->importer->getCardList().size()); + const int cardCount = wizard()->importer->getCardList().size(); + if (wizard()->backgroundMode) { + qInfo() << tr("Import finished: %1 cards.").arg(cardCount); emitBackgroundProgress("import", totalSets, totalSets); } else { - messageLog->append("" + tr("Import finished: %1 cards.").arg(wizard()->importer->getCardList().size()) + - ""); + messageLog->append("" + tr("Import finished: %1 cards.").arg(cardCount) + ""); } } else { progressBar->setValue(setIndex); - if (background) { + if (wizard()->backgroundMode) { qInfo() << tr("%1: %2 cards imported").arg(setName).arg(cardsImported); emitBackgroundProgress("import", setIndex, totalSets); } else { diff --git a/oracle/src/pages.h b/oracle/src/pages.h index b0543e98f..6417a7c0e 100644 --- a/oracle/src/pages.h +++ b/oracle/src/pages.h @@ -3,8 +3,10 @@ #include "pagetemplates.h" +#include #include #include +#include #include #include #include @@ -57,6 +59,16 @@ protected: void initializePage() override; }; +/** @brief Result of a worker-thread sets-file load (read + decompress + dispatch). */ +struct LoadSetsResult +{ + bool ok = false; ///< JSON scan produced set data (or plain XML was handled) + bool plainXml = false; ///< input was a plain Cockatrice XML database + QByteArray xmlData; ///< raw XML for the plain-XML path + QString errorMessage; ///< set when the input could not be processed + bool offerUncompressedFallback = false; ///< decompression-only failure: offer the uncompressed URL +}; + class LoadSetsPage : public OracleWizardPage { Q_OBJECT @@ -68,8 +80,9 @@ protected: void initializePage() override; bool validatePage() override; void readSetsFromByteArray(QByteArray _data); - void readSetsFromByteArrayRef(QByteArray &_data); + void readSetsFromFile(const QString &fileName); void downloadSetsFile(const QUrl &url); + void cancelWork() override; private: QRadioButton *urlRadioButton; @@ -81,8 +94,10 @@ private: QLabel *progressLabel; QProgressBar *progressBar; - QFutureWatcher watcher; - QFuture future; + QFutureWatcher watcher; + QFuture future; + + void beginLoadSets(bool compressedFile = false); private slots: void actLoadSetsFile(); @@ -111,11 +126,14 @@ private: QFutureWatcher importWatcher; QFuture importFuture; + int totalSets = 0; + bool importActive = false; protected: void initializePage() override; void cleanupPage() override; bool validatePage() override; + void cancelWork() override; private slots: void importFinished(); diff --git a/oracle/src/pagetemplates.h b/oracle/src/pagetemplates.h index 6e79c867e..ccad7ec62 100644 --- a/oracle/src/pagetemplates.h +++ b/oracle/src/pagetemplates.h @@ -20,6 +20,14 @@ public: } virtual void retranslateUi() = 0; + /** + * @brief Asks an active page to stop any background worker before the wizard + * (and its importer) can be torn down underneath it. Default is a no-op. + */ + virtual void cancelWork() + { + } + signals: void readyToContinue();