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 d288236d2..4a2caaab4 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 @@ -17,8 +17,10 @@ #include static constexpr int MAX_REQUESTS_PER_SEC = 10; -static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance -static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int MIN_HOST_QUOTA = 1; ///< Floor for the per-host request allowance +static constexpr qint64 QUOTA_RECOVER_MS = 60000; ///< Idle time before a reduced quota starts recovering +static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between individual network requests +static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), @@ -60,11 +62,18 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() pictureLoaderThread->start(QThread::LowPriority); moveToThread(pictureLoaderThread); + // QTimer value members are not QObject children, so moveToThread on the worker doesn't move + // them. They must live in the worker's thread to be started from the slot code that runs there. + requestTimer.moveToThread(pictureLoaderThread); + dispatchTimer.moveToThread(pictureLoaderThread); + connect(this, &CardPictureLoaderWorker::imageLoadEnqueued, this, &CardPictureLoaderWorker::handleImageLoadEnqueued); connect(&requestTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::resetRequestQuota); - requestTimer.setInterval(1000); - requestTimer.start(); + requestTimer.setInterval(static_cast(QUOTA_RESET_INTERVAL_MS)); + + connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest); + dispatchTimer.setInterval(DISPATCH_INTERVAL_MS); } CardPictureLoaderWorker::~CardPictureLoaderWorker() @@ -84,8 +93,8 @@ void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWor SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE && 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 + // A request that will be served from the disk cache never touches the network and therefore + // doesn't use up any of the rate limit, so it gets to skip the queue. makeRequest(url, worker); return; } @@ -107,10 +116,13 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture req.setHeader(QNetworkRequest::UserAgentHeader, QString("Cockatrice %1").arg(VERSION_STRING)); req.setRawHeader("Accept", "image/avif,image/webp,image/apng,image/,/*;q=0.8"); - bool useNetworkCache = - !picDownload && static_cast( - SettingsCache::instance().cacheStorage().getCardPictureLoaderCacheMethod()) == - CardPictureLoaderCacheMethod::CacheMethod::NETWORK_CACHE; + // 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); req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, useNetworkCache ? QNetworkRequest::AlwaysCache : QNetworkRequest::AlwaysNetwork); @@ -144,8 +156,29 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { - while (requestQuota > 0 && processSingleRequest()) { + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + return; + } + // Start lazily from the worker's own thread: QTimer must be started in the thread it lives in. + if (!requestTimer.isActive()) { + requestTimer.start(); + } + dispatchTimer.start(); +} + +void CardPictureLoaderWorker::dispatchQueuedRequest() +{ + if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + dispatchTimer.stop(); + return; + } + + if (processSingleRequest()) { --requestQuota; + } else { + // No queued host currently has allowance left in this second; wait for the quota reset. + dispatchTimer.stop(); } } 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 d1c519b7a..9f7fd9437 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 @@ -89,6 +89,9 @@ public slots: /** @brief Processes all queued requests respecting the request quota. */ void processQueuedRequests(); + /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ + void dispatchQueuedRequest(); + /** * @brief Processes a single queued request. * @return true if a request was processed, false if queue is empty. @@ -120,6 +123,7 @@ private: int requestQuota; ///< Remaining requests allowed per second QTimer requestTimer; ///< Timer to reset the request quota + QTimer dispatchTimer; ///< Timer pacing individual network requests QHash hostRequestQuota; ///< Sustained per-host request allowance QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited