[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.
This commit is contained in:
Lukas Brübach 2026-09-12 22:50:19 +02:00 committed by BruebachL
parent 7b82ca08da
commit 77ded1da97
3 changed files with 72 additions and 5 deletions

View file

@ -11,6 +11,7 @@
#include <QDirIterator>
#include <QFileInfo>
#include <QMainWindow>
#include <QMetaObject>
#include <QMovie>
#include <QNetworkRequest>
#include <QPainter>
@ -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<ExactCard> &cards)

View file

@ -60,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);
@ -83,7 +86,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)

View file

@ -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