From dc67aed05ea6fb4a61717f09e99e39b96068eeb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 6 Sep 2026 04:51:07 +0200 Subject: [PATCH 1/3] [PictureLoader] Pace requests and run the throttle timers on the worker thread Previously the whole backed-up queue was drained in a burst as soon as a request was enqueued, sending up to 10 requests back-to-back and then immediately re-filling the quota one second later. That hard-bursts a rate-limited API like Scryfall's (10 requests/second) into a 30 second lockout. Introduce a pacing timer that dispatches a single queue entry every 100 ms, so the per-second allowance is used smoothly instead of in spikes, and keep the quota timer at 1 second. Also fix both timers' thread affinity: they are QTimer value members and so are not QObject children, meaning moveToThread() on the worker left them on the main thread while the slot code started them from the picture thread, which was a no-op that also warned. They are moved to the worker thread explicitly and started lazily from there. --- .../card_picture_loader_worker.cpp | 40 ++++++++++++++++--- .../card_picture_loader_worker.h | 4 ++ 2 files changed, 39 insertions(+), 5 deletions(-) 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 34092f361..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() @@ -147,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 From f9d2fd3c34c2baf1da1f70a3ca436b266a52e57c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Fri, 18 Sep 2026 03:55:45 +0200 Subject: [PATCH 2/3] [PictureLoader] Guard dispatch timer restarts and drop dead request quota --- .../card_picture_loader_worker.cpp | 22 +++++++++++-------- .../card_picture_loader_worker.h | 3 +-- 2 files changed, 14 insertions(+), 11 deletions(-) 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 4a2caaab4..b29e838ff 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 @@ -23,8 +23,7 @@ static constexpr int DISPATCH_INTERVAL_MS = 100; ///< Pacing between indi static constexpr qint64 QUOTA_RESET_INTERVAL_MS = 1000; ///< Interval at which the request quota resets CardPictureLoaderWorker::CardPictureLoaderWorker() - : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - requestQuota(MAX_REQUESTS_PER_SEC) + : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -137,8 +136,6 @@ QNetworkReply *CardPictureLoaderWorker::makeRequest(const QUrl &url, CardPicture void CardPictureLoaderWorker::resetRequestQuota() { - requestQuota = MAX_REQUESTS_PER_SEC; - QDateTime now = QDateTime::currentDateTime(); for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { @@ -156,27 +153,34 @@ void CardPictureLoaderWorker::resetRequestQuota() void CardPictureLoaderWorker::processQueuedRequests() { + Q_ASSERT(thread() == QThread::currentThread()); + if (requestLoadQueue.isEmpty()) { dispatchTimer.stop(); + requestTimer.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(); + // Restarting an active timer would reset the pacing countdown, so a burst of enqueues could + // keep starving the dispatcher; only start it when it has actually stopped. + if (!dispatchTimer.isActive()) { + dispatchTimer.start(); + } } void CardPictureLoaderWorker::dispatchQueuedRequest() { - if (requestLoadQueue.isEmpty() || requestQuota <= 0) { + if (requestLoadQueue.isEmpty()) { + // All queued requests have been dispatched; stop the pacing and quota-reset timers. dispatchTimer.stop(); + requestTimer.stop(); return; } - if (processSingleRequest()) { - --requestQuota; - } else { + if (!processSingleRequest()) { // 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 9f7fd9437..f0fe17976 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 @@ -86,7 +86,7 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Processes all queued requests respecting the request quota. */ + /** @brief Starts the pacing timers if there is queued work, stops them when the queue is empty. */ void processQueuedRequests(); /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ @@ -121,7 +121,6 @@ private: bool picDownload; ///< Whether downloading images from network is enabled QQueue> requestLoadQueue; ///< Queue of pending network requests - 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 From 04dea3061b9808692688c98dbf021e3b55c74787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 20 Sep 2026 21:24:40 +0200 Subject: [PATCH 3/3] [PictureLoader] Reconcile quota-timer lifecycle with idle 429 recovery --- .../card_picture_loader_worker.cpp | 60 +++++++++++++------ .../card_picture_loader_worker.h | 5 +- 2 files changed, 46 insertions(+), 19 deletions(-) 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 b29e838ff..728c5cc6d 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 @@ -148,35 +148,25 @@ void CardPictureLoaderWorker::resetRequestQuota() hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC)); } - processQueuedRequests(); + updateTimerState(); } void CardPictureLoaderWorker::processQueuedRequests() { - Q_ASSERT(thread() == QThread::currentThread()); - - if (requestLoadQueue.isEmpty()) { - dispatchTimer.stop(); - requestTimer.stop(); + // QTimer must be started from the thread it lives in; if this public slot is ever reached from + // another thread, replay it on the worker's event loop instead of letting start() fail silently. + if (thread() != QThread::currentThread()) { + QMetaObject::invokeMethod(this, &CardPictureLoaderWorker::processQueuedRequests, Qt::QueuedConnection); return; } - // Start lazily from the worker's own thread: QTimer must be started in the thread it lives in. - if (!requestTimer.isActive()) { - requestTimer.start(); - } - // Restarting an active timer would reset the pacing countdown, so a burst of enqueues could - // keep starving the dispatcher; only start it when it has actually stopped. - if (!dispatchTimer.isActive()) { - dispatchTimer.start(); - } + updateTimerState(); } void CardPictureLoaderWorker::dispatchQueuedRequest() { if (requestLoadQueue.isEmpty()) { - // All queued requests have been dispatched; stop the pacing and quota-reset timers. - dispatchTimer.stop(); - requestTimer.stop(); + // All queued requests have been dispatched; stop the pacing timers. + updateTimerState(); return; } @@ -186,6 +176,40 @@ void CardPictureLoaderWorker::dispatchQueuedRequest() } } +void CardPictureLoaderWorker::updateTimerState() +{ + // Never restart an active timer: that would reset the pacing countdown and a burst of enqueues + // could keep starving the dispatcher, so only (re)start a timer that has actually stopped. + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + // Forget per-second allowances once nothing is pending: a stale zero would otherwise delay + // the next single request by a full quota-reset interval. + hostQuotaRemaining.clear(); + } else if (!dispatchTimer.isActive()) { + dispatchTimer.start(); + } + + // The quota timer resets allowances every second and is also the only thing that heals a host + // after a 429 (see resetRequestQuota). It must keep ticking while work is queued or a host is + // still recovering below the ceiling, and only winds down once no host needs recovery anymore. + // Keeping it alive during such idle periods lets reduced quotas recover as intended. + bool hostRecovering = false; + for (auto it = hostRequestQuota.cbegin(); it != hostRequestQuota.cend(); ++it) { + if (it.value() < MAX_REQUESTS_PER_SEC) { + hostRecovering = true; + break; + } + } + + if (!requestLoadQueue.isEmpty() || hostRecovering) { + if (!requestTimer.isActive()) { + requestTimer.start(); + } + } else if (requestTimer.isActive()) { + requestTimer.stop(); + } +} + bool CardPictureLoaderWorker::processSingleRequest() { for (int i = 0; i < requestLoadQueue.size(); ++i) { 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 f0fe17976..2a13e847c 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 @@ -86,7 +86,7 @@ public slots: */ QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread); - /** @brief Starts the pacing timers if there is queued work, stops them when the queue is empty. */ + /** @brief Ensures the pacing and quota-reset timers reflect the current queue and recovery state. */ void processQueuedRequests(); /** @brief Chooses a request from the queue and starts it, respecting the quota and pacing. */ @@ -142,6 +142,9 @@ private: /** @brief Removes stale redirect entries older than TTL. */ void cleanStaleEntries(); + /** @brief Starts or stops the pacing and quota-reset timers to match the queue and recovery state. */ + void updateTimerState(); + private slots: /** @brief Resets the request quota for rate-limiting. */ void resetRequestQuota();