Merge a15ae91aa7 into 659e30f251

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp

@@ -107,6 +107,14 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture
     // Check for cached redirects
     QUrl cachedRedirect = getCachedRedirect(url);
     if (!cachedRedirect.isEmpty()) {
+        // The redirect target is a different host, which may itself be in 429 backoff; hand the
+        // entry back to its worker so it waits the backoff out instead of dispatching straight
+        // onto the backed-off host.
+        if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(),
+                                                                     QDateTime::currentDateTime())) {
+            worker->scheduleDeferredRetry(cachedRedirect.host());
+            return nullptr;
+        }
         emit imageRequestSucceeded(url);
         return makeRequest(cachedRedirect, worker);
     }
@@ -143,10 +151,10 @@ void CardPictureLoaderWorker::resetRequestQuota()
         }
     }
 
-    for (const auto &request : requestLoadQueue) {
-        const QString host = request.first.host();
-        hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC));
-    }
+    // Forget the per-second allowances; each host's allowance is re-seeded lazily from its
+    // reduced sustained quota the first time it is dispatched in the new second, so a host that
+    // enters the queue mid-second no longer falls through to a fresh full quota.
+    hostQuotaRemaining.clear();
 
     updateTimerState();
 }
@@ -212,14 +220,28 @@ void CardPictureLoaderWorker::updateTimerState()
 
 bool CardPictureLoaderWorker::processSingleRequest()
 {
+    QDateTime now = QDateTime::currentDateTime();
     for (int i = 0; i < requestLoadQueue.size(); ++i) {
         const auto &request = requestLoadQueue.at(i);
-        QString host = request.first.host();
-        int allowance = hostQuotaRemaining.value(host, MAX_REQUESTS_PER_SEC);
+        const QString host = request.first.host();
+        // Don't dispatch requests to a host that is currently in its 429 backoff; hand the entry
+        // back to its worker so it can wait the backoff out or fall through to another source,
+        // instead of leaving it parked in the queue with no reply pending.
+        if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) {
+            auto entry = requestLoadQueue.takeAt(i);
+            entry.second->startNextPicDownload();
+            return true;
+        }
+        // Seed the allowance now so a host that was rate limited gets its reduced
+        // allowance instead of a fresh full quota mid-second.
+        if (!hostQuotaRemaining.contains(host)) {
+            hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC));
+        }
+        int allowance = hostQuotaRemaining.value(host);
         if (allowance > 0) {
             hostQuotaRemaining.insert(host, allowance - 1);
-            makeRequest(request.first, request.second);
-            requestLoadQueue.removeAt(i);
+            auto entry = requestLoadQueue.takeAt(i);
+            makeRequest(entry.first, entry.second);
             return true;
         }
     }

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp

@@ -22,6 +22,11 @@ static const QStringList MD5_BLACKLIST = {
     "fbc7d763c08771c260b39e2115414eeb"  // Current card back hash
 };
 
+const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter()
+{
+    return s_rateLimiter;
+}
+
 CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad)
     : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)),
       picDownload(SettingsCache::instance().downloads().getPicDownload())
@@ -168,7 +173,7 @@ void CardPictureLoaderWorkerWork::handleFailedReply(const QNetworkReply *reply)
                 << "PictureLoader: [card: " << cardToDownload.getCard().getName()
                 << " set: " << cardToDownload.getSetName() << "]: Too many requests from " << host
                 << ", backing off until " << backoffUntil.toString(Qt::ISODate) << ", retrying the same url";
-            scheduleDeferredRetry();
+            scheduleDeferredRetry(host);
         } else {
             qCWarning(CardPictureLoaderWorkerWorkLog).nospace()
                 << "PictureLoader: [card: " << cardToDownload.getCard().getName()
@@ -273,14 +278,16 @@ QImage CardPictureLoaderWorkerWork::tryLoadImageFromReply(QNetworkReply *reply)
     return imgReader.read();
 }
 
-void CardPictureLoaderWorkerWork::scheduleDeferredRetry()
+void CardPictureLoaderWorkerWork::scheduleDeferredRetry(const QString &preferredHost)
 {
     QDateTime now = QDateTime::currentDateTime();
 
-    // Prefer waiting on the current URL's server so we retry the same source.
-    QString currentHost = QUrl(cardToDownload.getCurrentUrl()).host();
-    QDateTime backoffUntil = s_rateLimiter.deadline(currentHost);
-    if (!s_rateLimiter.isRateLimited(currentHost, now)) {
+    // Prefer waiting on the server that is actually blocking the request: callers hand in the
+    // rate-limited host when it differs from the current URL (e.g. a cached redirect target still
+    // in backoff), otherwise fall back to the current URL's server so we retry the same source.
+    QString waitHost = preferredHost.isEmpty() ? QUrl(cardToDownload.getCurrentUrl()).host() : preferredHost;
+    QDateTime backoffUntil = s_rateLimiter.deadline(waitHost);
+    if (!s_rateLimiter.isRateLimited(waitHost, now)) {
         backoffUntil = s_rateLimiter.earliestDeadline(now);
     }
 

cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h

@@ -10,6 +10,7 @@
 #include <QNetworkAccessManager>
 #include <QObject>
 #include <QRandomGenerator>
+#include <QString>
 #include <QThread>
 #include <libcockatrice/card/database/card_database.h>
 #include <libcockatrice/utility/server_rate_limiter.h>
@@ -43,6 +44,30 @@ public:
 
     CardPictureToLoad cardToDownload; ///< The card and associated URLs to try downloading
 
+    /** @brief Shared per-server 429 backoff state. */
+    static const ServerRateLimiter &rateLimiter();
+
+    /**
+     * @brief Starts downloading the next URL for this card.
+     *
+     * Skips URLs whose server is currently in 429 backoff, either waiting the
+     * backoff out or falling through to the other configured sources. Also used by
+     * the dispatch machinery to hand an entry back after it was removed from the
+     * request queue when its host turned out to be backed off.
+     */
+    void startNextPicDownload();
+
+    /**
+     * @brief Schedules a deferred retry after the relevant server backoff expires.
+     * @param preferredHost The server that is actually blocking the request, or an empty
+     *                      string to use the current URL's server
+     *
+     * Waits on the blocking server's backoff deadline, otherwise on the earliest active
+     * backoff. If no servers are in backoff, concludes with failure. Otherwise resets the
+     * CardPictureToLoad indices and retries after the backoff period.
+     */
+    void scheduleDeferredRetry(const QString &preferredHost = {});
+
 public slots:
     /**
      * @brief Handles a finished network reply for the card image.
@@ -55,9 +80,6 @@ private:
 
     static ServerRateLimiter s_rateLimiter; ///< Shared per-server 429 backoff state
 
-    /** @brief Starts downloading the next URL for this card. */
-    void startNextPicDownload();
-
     /** @brief Called when all URLs have been exhausted or download failed. */
     void picDownloadFailed();
 
@@ -82,16 +104,6 @@ private:
      */
     void concludeImageLoad(const QImage &image);
 
-    /**
-     * @brief Schedules a deferred retry after the relevant server backoff expires.
-     *
-     * Waits on the current URL's server when it is the reason we are blocked,
-     * otherwise on the earliest active backoff. If no servers are in backoff,
-     * concludes with failure. Otherwise resets the CardPictureToLoad indices and
-     * retries after the backoff period.
-     */
-    void scheduleDeferredRetry();
-
 private slots:
     /** @brief Updates the picDownload setting when it changes. */
     void picDownloadChanged();