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 d8779863e..93ce6c96f 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 @@ -107,6 +107,9 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // Check for cached redirects QUrl cachedRedirect = getCachedRedirect(url); if (!cachedRedirect.isEmpty()) { + // The status bar still needs to reclaim this URL's widget even when we hand the request back + // for a deferred retry instead of dispatching it onto the network. + emit imageRequestSucceeded(url); // 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. @@ -115,7 +118,6 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture worker->scheduleDeferredRetry(cachedRedirect.host()); return nullptr; } - emit imageRequestSucceeded(url); return makeRequest(cachedRedirect, worker); } @@ -126,10 +128,7 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture // Cached entries are served straight from the disk cache even when picture downloads are // enabled: re-fetching an already-cached image would burn the rate limit for nothing. Only a // genuine cache miss goes to the network, and only when downloads are enabled. - bool useNetworkCache = static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && - (cache->metaData(url).isValid() || !picDownload); + bool useNetworkCache = !requestTouchesNetwork(url); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); @@ -226,11 +225,19 @@ bool CardPictureLoaderWorker::processSingleRequest() 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)) { + // instead of leaving it parked in the queue with no reply pending. Only applies to + // requests that will actually touch the network: one that will be served from the disk + // cache costs nothing and shouldn't wait out the 429. + if (requestTouchesNetwork(request.first) && + CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + // The queued URL is usually a cached-redirect target whose host differs from + // cardToDownload.getCurrentUrl(), so scheduleDeferredRetry() (which waits out the + // blocked host's deadline) is used instead of startNextPicDownload() looping on the + // original host. Keep scanning so one backed-off entry doesn't monopolize the tick. auto entry = requestLoadQueue.takeAt(i); - entry.second->startNextPicDownload(); - return true; + --i; + entry.second->scheduleDeferredRetry(host); + continue; } // Seed the allowance now so a host that was rate limited gets its reduced // allowance instead of a fresh full quota mid-second. @@ -239,15 +246,27 @@ bool CardPictureLoaderWorker::processSingleRequest() } int allowance = hostQuotaRemaining.value(host); if (allowance > 0) { - hostQuotaRemaining.insert(host, allowance - 1); auto entry = requestLoadQueue.takeAt(i); - makeRequest(entry.first, entry.second); + // The allowance is only spent when a request is actually issued: makeRequest() returns + // nullptr when the cached redirect target is in backoff and it hands the entry back. + if (makeRequest(entry.first, entry.second)) { + hostQuotaRemaining.insert(host, allowance - 1); + } return true; } } return false; } +bool CardPictureLoaderWorker::requestTouchesNetwork(const QUrl &url) const +{ + bool useNetworkCache = static_cast( + SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == + CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && + (cache->metaData(url).isValid() || !picDownload); + return !useNetworkCache; +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); 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 2a13e847c..c6a93cb1c 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 @@ -133,6 +133,10 @@ private: /** @brief Returns cached redirect URL for the given original URL, if available. */ [[nodiscard]] QUrl getCachedRedirect(const QUrl &originalUrl) const; + /** @brief Whether a request for this URL would actually touch the network, rather than being served from the disk + * cache. */ + [[nodiscard]] bool requestTouchesNetwork(const QUrl &url) const; + /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h index c5aa07d10..b5cd57af8 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.h @@ -51,9 +51,7 @@ public: * @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. + * backoff out or falling through to the other configured sources. */ void startNextPicDownload();