mirror of
https://github.com/Cockatrice/Cockatrice.git
synced 2026-09-22 01:25:10 -07:00
[PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff (#7286)
* [PictureLoader] Seed per-host allowances on demand and skip hosts in 429 backoff The quota reset re-filled every host's remaining allowance to a full MAX_REQUESTS_PER_SEC as soon as the queue had a request for it, so a server that was just rate limited could be hammered again at full speed immediately after (or even during) recovery. Only seed a host's allowance the first time it is dispatched in the current second, seeded from its reduced sustained quota, and skip hosts still inside their 429 backoff window entirely, handing the entry back to its worker so it can wait the backoff out or fall through to another source instead of parking in the queue with no reply pending. Deferrals wait on the host that is actually blocking the request (cached-redirect targets and the reply host of a 429) rather than the current card URL's host. Rebased onto network-requests/request-pacing, which absorbed the earlier pacing and dispatch-guard commits, and reuses its updateTimerState idle-429 recovery plumbing. * [PictureLoader] Hand backed-off entries back across the whole dispatch tick Review fixes on the 429-backoff skip: - Hand a backed-off queue entry back to its worker via scheduleDeferredRetry(host) instead of startNextPicDownload(), which was looping on the pre-redirect host every 100ms tick whenever the queued URL was a cached-redirect target whose redirect host was the one in backoff. - Keep scanning the queue after a hand-back (--i; continue) so a backed-off host at the head no longer consumes the entire dispatch tick, stalling every healthy host further down. - Emit imageRequestSucceeded(url) before makeRequest()'s cached-redirect backoff return so the status bar reclaims the deferred URL's widget instead of inflating the progress bar forever. - Only spend a per-second allowance when makeRequest() actually issues a request: it returns nullptr when the redirect target is backed off and the work is handed back, so the slot would otherwise be wasted on a no-op. - Gate the dispatch-time backoff skip on requestTouchesNetwork() so entries served straight from the disk cache (e.g. with downloads disabled) are not bounced into a 30-60s deferred retry for a host that 429'd earlier. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
This commit is contained in:
BruebachL
GitHub
parent
334a743953
commit
3d5eb84d81
4 changed files with 94 additions and 32 deletions
|
|
@ -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<CardPictureLoaderCacheMethod::CacheMethod>(
|
||||
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<CardPictureLoaderCacheMethod::CacheMethod>(
|
||||
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));
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@
|
|||
#include <QNetworkAccessManager>
|
||||
#include <QObject>
|
||||
#include <QRandomGenerator>
|
||||
#include <QString>
|
||||
#include <QThread>
|
||||
#include <libcockatrice/card/database/card_database.h>
|
||||
#include <libcockatrice/utility/server_rate_limiter.h>
|
||||
|
|
@ -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();
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue