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 728c5cc6d..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,7 +107,17 @@ 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. + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(cachedRedirect.host(), + QDateTime::currentDateTime())) { + worker->scheduleDeferredRetry(cachedRedirect.host()); + return nullptr; + } return makeRequest(cachedRedirect, worker); } @@ -118,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); @@ -143,10 +150,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,20 +219,54 @@ 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. 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); + --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. + 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); + // 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.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp index 66c56337c..b70207ff4 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp +++ b/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); } 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 1e56a4373..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 @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -43,6 +44,28 @@ 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. + */ + 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 +78,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 +102,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();