[PictureLoader] Guard cache teardown and stop blocking the UI thread

cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp

@@ -44,6 +44,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr)
 
     qRegisterMetaType<ExactCard>("ExactCard");
     connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded);
+    connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared);
 
     statusBar = new CardPictureLoaderStatusBar(nullptr);
     QMainWindow *mainWindow = qobject_cast<QMainWindow *>(QApplication::activeWindow());
@@ -63,11 +64,15 @@ 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;
+        // 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;
         }
     }
+}
 
 void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size)
 {
@@ -465,15 +470,17 @@ void CardPictureLoader::clearPixmapCache()
 
 void CardPictureLoader::clearNetworkCache()
 {
-    auto &worker = *getInstance().worker;
+    // During teardown the worker is released before this singleton, so a queued clear may still
-    // The disk cache and redirect cache are owned by the worker thread; clearing them from the
+    // arrive with no worker left to run it.
-    // UI thread would race with the worker's cache reads/writes. Block until the worker thread
+    CardPictureLoaderWorker *worker = getInstance().worker;
-    // has executed the clear so the "Cached card pictures have been reset." message is truthful.
+    if (!worker) {
-    if (worker.isRunning()) {
+        return;
-        QMetaObject::invokeMethod(&worker, "clearNetworkCache", Qt::BlockingQueuedConnection);
-    } else {
-        worker.clearNetworkCache();
     }
+    // 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<ExactCard> &cards)

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.

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) {
+    if (!thread) {
-        thread->quit();
+        return true;
-        thread->wait();
     }
+    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();
 }

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
+     * exit request to the worker's event loop and QThread::wait() blocks (bounded) until the loop
-     * returned and the thread finished. Only QThread members are touched here, so this method is
+     * has returned and the thread finished. Only QThread members are touched here, so this method
-     * safe to call from the owning thread. The worker object itself is freed by the finished() ->
+     * 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
+     * -> 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();
+    [[nodiscard]] bool shutdownThread();
-
-    /** @return Whether the worker's thread is currently running. */
-    bool isRunning() const;
 
     /**
      * @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

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)
+CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad)
-    : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)),
+    : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)),
       picDownload(SettingsCache::instance().downloads().getPicDownload())
 {
     // Hook up signals to the orchestrator

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
 

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()