From a28ab1112643f58c30f7d64c15e902a07b5a6100 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 12 Sep 2026 22:50:19 +0200 Subject: [PATCH 1/2] [PictureLoader] Fix worker thread shutdown and cross-thread cache clearing clearNetworkCache() ran directly on the UI thread while the worker thread owned the disk cache and redirect cache, racing cache reads/writes. Make it a worker-thread slot invoked via a blocking queued call when the thread is running, so the 'Cached card pictures have been reset.' message is truthful. The worker thread was also never quit()/wait()ed: both destructors only deleteLater'd their objects, so Qt warned 'QThread: Destroyed while thread is still running' and leaked a running loop at exit. Wire the worker's finished() signal to its own deleteLater() (canonical worker-object pattern), add shutdownThread() to stop the loop, and let CardPictureLoader destroy the QThread only after wait() has returned. --- .../card_picture_loader.cpp | 20 ++++++++++-- .../card_picture_loader_worker.cpp | 26 +++++++++++++++- .../card_picture_loader_worker.h | 31 +++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index b8a54761a..e2332bed8 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -58,7 +59,14 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - worker->deleteLater(); + if (worker) { + // Capture the thread first: shutdownThread() blocks until the worker has been freed by the + // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. + QThread *pictureLoaderThread = worker->workerThread(); + worker->shutdownThread(); + worker = nullptr; + delete pictureLoaderThread; + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -457,7 +465,15 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + auto &worker = *getInstance().worker; + // The disk cache and redirect cache are owned by the worker thread; clearing them from the + // UI thread would race with the worker's cache reads/writes. Block until the worker thread + // has executed the clear so the "Cached card pictures have been reset." message is truthful. + if (worker.isRunning()) { + QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); + } else { + worker.clearNetworkCache(); + } } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 5bb513809..31c037378 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -59,6 +59,9 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; + // The worker object frees itself once its thread finishes, so no event loop is left + // running and the QThread is never destroyed while still executing. + connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); @@ -82,7 +85,28 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +void CardPictureLoaderWorker::shutdownThread() +{ + // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as + // its event loop exits, so nothing - not even a member read - may run once wait() returns. + // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. + QThread *thread = pictureLoaderThread; + if (thread) { + thread->quit(); + thread->wait(); + } +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; +} + +bool CardPictureLoaderWorker::isRunning() const +{ + return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index b720247cb..c1a71d36c 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -74,10 +74,37 @@ public: */ void onHostRateLimited(const QString &host); - /** @brief Clears the network cache and redirect cache. */ - void clearNetworkCache(); + /** + * @brief Stops the worker thread and releases it. + * + * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an + * exit request to the worker's event loop and QThread::wait() blocks until the loop has + * returned and the thread finished. Only QThread members are touched here, so this method is + * safe to call from the owning thread. The worker object itself is freed by the finished() -> + * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * owner (CardPictureLoader::~CardPictureLoader), not by this method. + */ + void shutdownThread(); + + /** @return Whether the worker's thread is currently running. */ + bool isRunning() const; + + /** + * @brief Returns the worker's QThread. + * @return The worker thread + * + * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). + */ + QThread *workerThread() const; public slots: + /** + * @brief Clears the network cache and redirect cache. + * + * Runs on the worker thread; invoke it via a queued call when coming from another thread, + * since both caches are owned by the worker thread. + */ + void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load From 0d2305617a8dfe7371e5cb766230858c12b21bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 04:42:31 +0200 Subject: [PATCH 2/2] [PictureLoader] Guard cache teardown and stop blocking the UI thread --- .../card_picture_loader.cpp | 27 ++++++++++++------- .../card_picture_loader/card_picture_loader.h | 7 +++++ .../card_picture_loader_worker.cpp | 25 ++++++++++------- .../card_picture_loader_worker.h | 21 ++++++++------- .../card_picture_loader_worker_work.cpp | 4 +-- .../card_picture_loader_worker_work.h | 5 ++-- .../settings_page/storage_settings_page.cpp | 9 +++++-- 7 files changed, 64 insertions(+), 34 deletions(-) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index e2332bed8..8e34942e1 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -44,6 +44,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) qRegisterMetaType("ExactCard"); connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded); + connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared); statusBar = new CardPictureLoaderStatusBar(nullptr); QMainWindow *mainWindow = qobject_cast(QApplication::activeWindow()); @@ -63,9 +64,13 @@ CardPictureLoader::~CardPictureLoader() // Capture the thread first: shutdownThread() blocks until the worker has been freed by the // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. QThread *pictureLoaderThread = worker->workerThread(); - worker->shutdownThread(); + const bool stopped = worker->shutdownThread(); worker = nullptr; - delete pictureLoaderThread; + // Deleting a QThread that is still running is undefined behaviour, so only free it once the + // bounded wait in shutdownThread() confirmed that it stopped. + if (stopped) { + delete pictureLoaderThread; + } } } @@ -465,15 +470,17 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - auto &worker = *getInstance().worker; - // The disk cache and redirect cache are owned by the worker thread; clearing them from the - // UI thread would race with the worker's cache reads/writes. Block until the worker thread - // has executed the clear so the "Cached card pictures have been reset." message is truthful. - if (worker.isRunning()) { - QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection); - } else { - worker.clearNetworkCache(); + // During teardown the worker is released before this singleton, so a queued clear may still + // arrive with no worker left to run it. + CardPictureLoaderWorker *worker = getInstance().worker; + if (!worker) { + return; } + // The disk cache and redirect cache are owned by the worker thread, so the clear has to run + // there. Invoke it asynchronously to keep the GUI responsive while the worker may be walking + // the user's picture directories or recursively deleting the cache directory; callers that + // need to know when it is done can listen for networkCacheCleared(). + QMetaObject::invokeMethod(worker, &CardPictureLoaderWorker::clearNetworkCache, Qt::QueuedConnection); } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 0a4934e6d..6224befbf 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -117,6 +117,9 @@ public: public slots: /** * @brief Clears the network disk cache of the worker. + * + * The clear runs on the worker thread, so this returns before it has completed; connect to + * networkCacheCleared() to act once it is done. */ static void clearNetworkCache(); @@ -131,6 +134,10 @@ public slots: void installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard); void installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard); +signals: + /** @brief Emitted after the worker has finished clearing the network and redirect caches. */ + void networkCacheCleared(); + private slots: /** * @brief Triggered when the user changes the picture download settings. diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp index 31c037378..eed84f3a6 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp @@ -21,6 +21,7 @@ static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets +static constexpr int THREAD_SHUTDOWN_WAIT_MS = 5000; ///< Bounded wait for the worker thread to stop at exit CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), @@ -87,16 +88,26 @@ CardPictureLoaderWorker::~CardPictureLoaderWorker() saveRedirectCache(); } -void CardPictureLoaderWorker::shutdownThread() +bool CardPictureLoaderWorker::shutdownThread() { // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as // its event loop exits, so nothing - not even a member read - may run once wait() returns. // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. QThread *thread = pictureLoaderThread; - if (thread) { - thread->quit(); - thread->wait(); + if (!thread) { + return true; } + thread->quit(); + // Only an unbounded wait() would guarantee the thread stops, but this runs from a function-local + // static destructor after main() has returned, with no UI left to interrupt a worker stuck in a + // slow slot or on a stalled filesystem. Bound the wait and leave such a thread to the OS rather + // than hanging the process forever. + if (!thread->wait(THREAD_SHUTDOWN_WAIT_MS)) { + qCWarning(CardPictureLoaderWorkerLog) << "Picture loader worker thread did not stop within" + << THREAD_SHUTDOWN_WAIT_MS << "ms; leaving it to be torn down by the OS"; + return false; + } + return true; } QThread *CardPictureLoaderWorker::workerThread() const @@ -104,11 +115,6 @@ QThread *CardPictureLoaderWorker::workerThread() const return pictureLoaderThread; } -bool CardPictureLoaderWorker::isRunning() const -{ - return pictureLoaderThread != nullptr && pictureLoaderThread->isRunning(); -} - void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) { QUrl cachedRedirect = getCachedRedirect(url); @@ -518,4 +524,5 @@ void CardPictureLoaderWorker::clearNetworkCache() { networkManager->cache()->clear(); redirectCache.clear(); + emit networkCacheCleared(); } diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h index c1a71d36c..e2829c6ee 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h @@ -75,19 +75,19 @@ public: void onHostRateLimited(const QString &host); /** - * @brief Stops the worker thread and releases it. + * @brief Stops the worker thread and reports whether it stopped. * * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an - * exit request to the worker's event loop and QThread::wait() blocks until the loop has - * returned and the thread finished. Only QThread members are touched here, so this method is - * safe to call from the owning thread. The worker object itself is freed by the finished() -> - * deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * exit request to the worker's event loop and QThread::wait() blocks (bounded) until the loop + * has returned and the thread finished. Only QThread members are touched here, so this method + * is safe to call from the owning thread. The worker object itself is freed by the finished() + * -> deleteLater chain (see the constructor); the QThread object is deleted afterwards by the * owner (CardPictureLoader::~CardPictureLoader), not by this method. + * + * @return true if the thread stopped within the timeout, false if it is still running (in + * which case the owner must not delete the QThread). */ - void shutdownThread(); - - /** @return Whether the worker's thread is currently running. */ - bool isRunning() const; + [[nodiscard]] bool shutdownThread(); /** * @brief Returns the worker's QThread. @@ -225,6 +225,9 @@ signals: /** @brief Emitted when a network request successfully completes. */ void imageRequestSucceeded(const QUrl &url); + + /** @brief Emitted after clearNetworkCache() has finished clearing both caches. */ + void networkCacheCleared(); }; #endif // PICTURE_LOADER_WORKER_H diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index b70207ff4..19c6be469 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp @@ -27,8 +27,8 @@ const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() return s_rateLimiter; } -CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) - : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), +CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad) + : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) { // Hook up signals to the orchestrator diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index c5aa07d10..eb8b11a24 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -37,10 +37,11 @@ class CardPictureLoaderWorkerWork : public QObject public: /** * @brief Constructs a worker for downloading a specific card image. - * @param worker The orchestrating CardPictureLoaderWorker + * @param worker The orchestrating CardPictureLoaderWorker; the work object becomes its child so + * it is destroyed with the worker even if it never reaches concludeImageLoad(). * @param toLoad The ExactCard to download */ - explicit CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad); + explicit CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad); CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading diff --git a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp index 17838e501..4aad91115 100644 --- a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp @@ -181,9 +181,14 @@ StorageSettingsPage::StorageSettingsPage() void StorageSettingsPage::clearDownloadedPicsButtonClicked() { - CardPictureLoader::clearNetworkCache(); + // The network cache is cleared asynchronously on the worker thread, so wait for the completion + // signal before confirming; the in-memory pixmap cache is cleared synchronously right away. + connect( + &CardPictureLoader::getInstance(), &CardPictureLoader::networkCacheCleared, this, + [this] { QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); }, + Qt::SingleShotConnection); CardPictureLoader::clearPixmapCache(); - QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); + CardPictureLoader::clearNetworkCache(); } void StorageSettingsPage::clearImageBackupsButtonClicked()