[PictureLoader] Properly run reply processing on Work's thread (#6016)

* [PictureLoader] Properly run reply processing on Work's thread

* emit cachedImageHit first

* Remove unused fields

* Remove unused fields

* Fix double free requests from cache hit

If we hit a cached url, the request already gets to skip the queue.
By sending another free request once the cached request finishes, we're actually sending two free requests on each cache hit.

cockatrice/src/client/ui/picture_loader/picture_loader_worker.cpp

@@ -12,9 +12,6 @@
 #include <QThread>
 #include <utility>
 
-// Card back returned by gatherer when card is not found
-QStringList PictureLoaderWorker::md5Blacklist = QStringList() << "db0c48db407a907c16ade38de048a441";
-
 PictureLoaderWorker::PictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().getPicDownload())
 {
     networkManager = new QNetworkAccessManager(this);
@@ -68,6 +65,7 @@ void PictureLoaderWorker::queueRequest(const QUrl &url, PictureLoaderWorkerWork
     if (!cachedRedirect.isEmpty()) {
         queueRequest(cachedRedirect, worker);
     } else if (cache->metaData(url).isValid()) {
+        // If we hit a cached url, we get to make the request for free, since it won't contribute towards the rate-limit
         makeRequest(url, worker);
     } else {
         requestLoadQueue.append(qMakePair(url, worker));
@@ -92,34 +90,7 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
     QNetworkReply *reply = networkManager->get(req);
 
     // Connect reply handling
-    connect(reply, &QNetworkReply::finished, this, [this, reply, url, worker]() {
-        QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
-        if (redirectTarget.isValid()) {
-            QUrl redirectUrl = redirectTarget.toUrl();
-            if (redirectUrl.isRelative()) {
-                redirectUrl = url.resolved(redirectUrl);
-            }
-            cacheRedirect(url, redirectUrl);
-        }
-
-        if (reply->error() == QNetworkReply::NoError) {
-            worker->picDownloadFinished(reply);
-            emit imageLoadSuccessful(url, worker);
-
-            // If we hit a cached image, we get to make another request for free.
-            if (reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool()) {
-                if (!requestLoadQueue.isEmpty()) {
-                    auto request = requestLoadQueue.takeFirst();
-                    makeRequest(request.first, request.second);
-                }
-            }
-        } else if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 429) {
-            qInfo() << "Too many requests.";
-        } else {
-            worker->picDownloadFinished(reply);
-        }
-        reply->deleteLater();
-    });
+    connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); });
 
     return reply;
 }
@@ -127,10 +98,18 @@ QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url, PictureLoaderWo
 void PictureLoaderWorker::processQueuedRequests()
 {
     for (int i = 0; i < 10; i++) {
-        if (!requestLoadQueue.isEmpty()) {
-            auto request = requestLoadQueue.takeFirst();
-            makeRequest(request.first, request.second);
-        }
+        processSingleRequest();
+    }
+}
+
+/**
+ * Immediately processes a single queued request
+ */
+void PictureLoaderWorker::processSingleRequest()
+{
+    if (!requestLoadQueue.isEmpty()) {
+        auto request = requestLoadQueue.takeFirst();
+        makeRequest(request.first, request.second);
     }
 }
 
@@ -176,6 +155,11 @@ void PictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl &red
     // saveRedirectCache();
 }
 
+void PictureLoaderWorker::removedCachedUrl(const QUrl &url)
+{
+    networkManager->cache()->remove(url);
+}
+
 QUrl PictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const
 {
     if (redirectCache.contains(originalUrl)) {

cockatrice/src/client/ui/picture_loader/picture_loader_worker.h

@@ -38,11 +38,12 @@ public:
 public slots:
     QNetworkReply *makeRequest(const QUrl &url, PictureLoaderWorkerWork *workThread);
     void processQueuedRequests();
+    void processSingleRequest();
     void imageLoadedSuccessfully(const CardInfoPtr &card, const QImage &image);
+    void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
+    void removedCachedUrl(const QUrl &url);
 
 private:
-    static QStringList md5Blacklist;
-
     QThread *pictureLoaderThread;
     QNetworkAccessManager *networkManager;
     QNetworkDiskCache *cache;
@@ -56,7 +57,6 @@ private:
     PictureLoaderLocal *localLoader;
     QSet<CardInfoPtr> currentlyLoading; // for deduplication purposes
 
-    void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
     QUrl getCachedRedirect(const QUrl &originalUrl) const;
     void loadRedirectCache();
     void saveRedirectCache() const;

cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.cpp

@@ -19,10 +19,10 @@ PictureLoaderWorkerWork::PictureLoaderWorkerWork(const PictureLoaderWorker *work
     : QObject(nullptr), cardToDownload(toLoad), picDownload(SettingsCache::instance().getPicDownload())
 {
     // Hook up signals to the orchestrator
-    connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest,
-            Qt::QueuedConnection);
-    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully,
-            Qt::QueuedConnection);
+    connect(this, &PictureLoaderWorkerWork::requestImageDownload, worker, &PictureLoaderWorker::queueRequest);
+    connect(this, &PictureLoaderWorkerWork::urlRedirected, worker, &PictureLoaderWorker::cacheRedirect);
+    connect(this, &PictureLoaderWorkerWork::cachedUrlInvalidated, worker, &PictureLoaderWorker::removedCachedUrl);
+    connect(this, &PictureLoaderWorkerWork::imageLoaded, worker, &PictureLoaderWorker::imageLoadedSuccessfully);
 
     // Hook up signals to settings
     connect(&SettingsCache::instance(), SIGNAL(picDownloadChanged()), this, SLOT(picDownloadChanged()));
@@ -44,7 +44,6 @@ void PictureLoaderWorkerWork::startNextPicDownload()
     QString picUrl = cardToDownload.getCurrentUrl();
 
     if (picUrl.isEmpty()) {
-        downloadRunning = false;
         picDownloadFailed();
     } else {
         QUrl url(picUrl);
@@ -78,24 +77,51 @@ void PictureLoaderWorkerWork::picDownloadFailed()
     }
 }
 
+/**
+ *
+ * @param reply The reply. Takes ownership of the object
+ */
+void PictureLoaderWorkerWork::handleNetworkReply(QNetworkReply *reply)
+{
+    QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
+    if (redirectTarget.isValid()) {
+        QUrl url = reply->request().url();
+        QUrl redirectUrl = redirectTarget.toUrl();
+        if (redirectUrl.isRelative()) {
+            redirectUrl = url.resolved(redirectUrl);
+        }
+        emit urlRedirected(url, redirectUrl);
+    }
+
+    if (reply->error()) {
+        handleFailedReply(reply);
+    } else {
+        handleSuccessfulReply(reply);
+    }
+
+    reply->deleteLater();
+}
+
 static bool imageIsBlackListed(const QByteArray &picData)
 {
     QString md5sum = QCryptographicHash::hash(picData, QCryptographicHash::Md5).toHex();
     return MD5_BLACKLIST.contains(md5sum);
 }
 
-void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
+void PictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
 {
-    bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
+    if (reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt() == 429) {
+        qCWarning(PictureLoaderWorkerWorkLog) << "Too many requests.";
+    } else {
+        bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
 
-    if (reply->error()) {
         if (isFromCache) {
             qCDebug(PictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
                 << " set: " << cardToDownload.getSetName() << "]: Removing corrupted cache file for url "
                 << reply->url().toDisplayString() << " and retrying (" << reply->errorString() << ")";
 
-            networkManager->cache()->remove(reply->url());
+            emit cachedUrlInvalidated(reply->url());
 
             emit requestImageDownload(reply->url(), this);
         } else {
@@ -106,10 +132,12 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
 
             picDownloadFailed();
         }
-
-        reply->deleteLater();
-        return;
     }
+}
+
+void PictureLoaderWorkerWork::handleSuccessfulReply(QNetworkReply *reply)
+{
+    bool isFromCache = reply->attribute(QNetworkRequest::SourceIsFromCacheAttribute).toBool();
 
     // List of status codes from https://doc.qt.io/qt-6/qnetworkreply.html#redirected
     int statusCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
@@ -120,7 +148,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << "PictureLoader: [card: " << cardToDownload.getCard()->getName()
             << " set: " << cardToDownload.getSetName() << "]: following "
             << (isFromCache ? "cached redirect" : "redirect") << " to " << redirectUrl.toDisplayString();
-        reply->deleteLater();
         emit requestImageDownload(redirectUrl, this);
         return;
     }
@@ -134,7 +161,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName()
             << "]: Picture found, but blacklisted, will consider it as not found";
 
-        reply->deleteLater();
         picDownloadFailed();
         return;
     }
@@ -147,7 +173,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName() << "]: Possible " << (isFromCache ? "cached" : "downloaded")
             << " picture at " << reply->url().toDisplayString() << " could not be loaded: " << reply->errorString();
 
-        reply->deleteLater();
         picDownloadFailed();
     } else {
         qCDebug(PictureLoaderWorkerWorkLog).nospace()
@@ -155,7 +180,6 @@ void PictureLoaderWorkerWork::picDownloadFinished(QNetworkReply *reply)
             << " set: " << cardToDownload.getSetName() << "]: Image successfully "
             << (isFromCache ? "loaded from cached" : "downloaded from") << " url " << reply->url().toDisplayString();
 
-        reply->deleteLater();
         concludeImageLoad(image);
     }
 }

cockatrice/src/client/ui/picture_loader/picture_loader_worker_work.h

@@ -30,15 +30,16 @@ public:
     PictureToLoad cardToDownload;
 
 public slots:
-    void picDownloadFinished(QNetworkReply *reply);
+    void handleNetworkReply(QNetworkReply *reply);
 
 private:
     QThread *pictureLoaderThread;
-    QNetworkAccessManager *networkManager;
-    bool picDownload, downloadRunning, loadQueueRunning;
+    bool picDownload;
 
     void startNextPicDownload();
     void picDownloadFailed();
+    void handleFailedReply(const QNetworkReply *reply);
+    void handleSuccessfulReply(QNetworkReply *reply);
     QImage tryLoadImageFromReply(QNetworkReply *reply);
     void concludeImageLoad(const QImage &image);
 
@@ -53,6 +54,9 @@ signals:
      */
     void imageLoaded(CardInfoPtr card, const QImage &image);
     void requestImageDownload(const QUrl &url, PictureLoaderWorkerWork *instance);
+
+    void urlRedirected(const QUrl &originalUrl, const QUrl &redirectUrl);
+    void cachedUrlInvalidated(const QUrl &url);
 };
 
 #endif // PICTURE_LOADER_WORKER_WORK_H