Merge 0d2305617a into c667de464a

cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp

@@ -11,6 +11,7 @@
 #include <QDir>
 #include <QFileInfo>
 #include <QMainWindow>
+#include <QMetaObject>
 #include <QMovie>
 #include <QNetworkRequest>
 #include <QPainter>
@@ -43,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());
@@ -58,7 +60,18 @@ 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();
+        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)
@@ -457,7 +470,17 @@ void CardPictureLoader::clearPixmapCache()
 
 void CardPictureLoader::clearNetworkCache()
 {
-    getInstance().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<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()),
@@ -59,6 +60,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 +86,33 @@ CardPictureLoaderWorker::CardPictureLoaderWorker()
 CardPictureLoaderWorker::~CardPictureLoaderWorker()
 {
     saveRedirectCache();
-    pictureLoaderThread->deleteLater();
+}
+
+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) {
+        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
+{
+    return pictureLoaderThread;
 }
 
 void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker)
@@ -494,4 +524,5 @@ void CardPictureLoaderWorker::clearNetworkCache()
 {
     networkManager->cache()->clear();
     redirectCache.clear();
+    emit networkCacheCleared();
 }

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 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 (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).
+     */
+    [[nodiscard]] bool shutdownThread();
+
+    /**
+     * @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
@@ -198,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)
-    : 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

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