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 8c81d641d..e7e31f5a2 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 @@ -55,7 +56,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) @@ -295,7 +303,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 5ab097bc4..a5965017e 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 1f4bebb53..d2a444ac9 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