jeirmeister/Cockatrice
1
0
Fork 0
mirror of https://github.com/Cockatrice/Cockatrice.git synced 2026-09-21 17:15:09 -07:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

[PictureLoader] Pace requests and run the throttle timers on the worker thread (#7285)

Browse source 
* [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.

* [PictureLoader] Guard dispatch timer restarts and drop dead request quota

* [PictureLoader] Reconcile quota-timer lifecycle with idle 429 recovery

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
This commit is contained in:
BruebachL 2026-09-21 18:30:02 +02:00 • committed by GitHub
parent e2a4556546
commit 334a743953
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 77 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

80
cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp
View file

@ -17,12 +17,13 @@
#include <version_string.h>
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()),
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
@ -60,11 +61,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<int>(QUOTA_RESET_INTERVAL_MS));
connect(&dispatchTimer, &QTimer::timeout, this, &CardPictureLoaderWorker::dispatchQueuedRequest);
dispatchTimer.setInterval(DISPATCH_INTERVAL_MS);
}
CardPictureLoaderWorker::~CardPictureLoaderWorker()
@ -128,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) {
@ -142,13 +148,65 @@ void CardPictureLoaderWorker::resetRequestQuota()
hostQuotaRemaining.insert(host, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC));
}
processQueuedRequests();
updateTimerState();
}
void CardPictureLoaderWorker::processQueuedRequests()
{
while (requestQuota > 0 && processSingleRequest()) {
--requestQuota;
// 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;
}
updateTimerState();
}
void CardPictureLoaderWorker::dispatchQueuedRequest()
{
if (requestLoadQueue.isEmpty()) {
// All queued requests have been dispatched; stop the pacing timers.
updateTimerState();
return;
}
if (!processSingleRequest()) {
// No queued host currently has allowance left in this second; wait for the quota reset.
dispatchTimer.stop();
}
}
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();
}
}

10
cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.h
View file

@ -86,9 +86,12 @@ public slots:
*/
QNetworkReply *makeRequest(const QUrl &url, CardPictureLoaderWorkerWork *workThread);
/** @brief Processes all queued requests respecting the request quota. */
/** @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. */
void dispatchQueuedRequest();
/**
* @brief Processes a single queued request.
* @return true if a request was processed, false if queue is empty.
@ -118,8 +121,8 @@ private:
bool picDownload; ///< Whether downloading images from network is enabled
QQueue<QPair<QUrl, CardPictureLoaderWorkerWork *>> 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<QString, int> hostRequestQuota; ///< Sustained per-host request allowance
QHash<QString, int> hostQuotaRemaining; ///< Per-host allowance left in the current second
QHash<QString, QDateTime> hostLast429; ///< When each host was last rate limited
@ -139,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();