diff --git a/cockatrice/src/client/settings/cache_settings.h b/cockatrice/src/client/settings/cache_settings.h index 23cdb4dbf..7a90d82f1 100644 --- a/cockatrice/src/client/settings/cache_settings.h +++ b/cockatrice/src/client/settings/cache_settings.h @@ -58,33 +58,33 @@ signals: void themeChanged(); private: - QSettings *settings; - ShortcutsSettings *shortcutsSettings; - CardDatabaseSettings *cardDatabaseSettings; - ServersSettings *serversSettings; - MessageSettings *messageSettings; - GameFiltersSettings *gameFiltersSettings; - LayoutsSettings *layoutsSettings; - DownloadSettings *downloadSettings; - RecentsSettings *recentsSettings; - CardOverrideSettings *cardOverrideSettings; - DebugSettings *debugSettings; - CardCounterSettings *cardCounterSettings; - TabsSettings *tabsSettings; - SoundSettings *soundSettings; - GameSettings *gameSettings; - ChatSettings *chatSettings; - CacheStorageSettings *cacheStorageSettings; - UpdatesSettings *updatesSettings; - PersonalSettings *personalSettings; - CardsDisplaySettings *cardsDisplaySettings; - InterfaceSettings *interfaceSettings; - DeckEditorSettings *deckEditorSettings; - PathsSettings *pathsSettings; - VisualDeckStorageSettings *visualDeckStorageSettings; - AppearanceSettings *appearanceSettings; - NetworkSettings *networkSettings; - CommanderBracketSettings *commanderBracketSettings; + QSettings *settings = nullptr; + ShortcutsSettings *shortcutsSettings = nullptr; + CardDatabaseSettings *cardDatabaseSettings = nullptr; + ServersSettings *serversSettings = nullptr; + MessageSettings *messageSettings = nullptr; + GameFiltersSettings *gameFiltersSettings = nullptr; + LayoutsSettings *layoutsSettings = nullptr; + DownloadSettings *downloadSettings = nullptr; + RecentsSettings *recentsSettings = nullptr; + CardOverrideSettings *cardOverrideSettings = nullptr; + DebugSettings *debugSettings = nullptr; + CardCounterSettings *cardCounterSettings = nullptr; + TabsSettings *tabsSettings = nullptr; + SoundSettings *soundSettings = nullptr; + GameSettings *gameSettings = nullptr; + ChatSettings *chatSettings = nullptr; + CacheStorageSettings *cacheStorageSettings = nullptr; + UpdatesSettings *updatesSettings = nullptr; + PersonalSettings *personalSettings = nullptr; + CardsDisplaySettings *cardsDisplaySettings = nullptr; + InterfaceSettings *interfaceSettings = nullptr; + DeckEditorSettings *deckEditorSettings = nullptr; + PathsSettings *pathsSettings = nullptr; + VisualDeckStorageSettings *visualDeckStorageSettings = nullptr; + AppearanceSettings *appearanceSettings = nullptr; + NetworkSettings *networkSettings = nullptr; + CommanderBracketSettings *commanderBracketSettings = nullptr; QString themeName; diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp index b8a54761a..8e34942e1 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -43,6 +44,7 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) qRegisterMetaType("ExactCard"); connect(worker, &CardPictureLoaderWorker::imageLoaded, this, &CardPictureLoader::imageLoaded); + connect(worker, &CardPictureLoaderWorker::networkCacheCleared, this, &CardPictureLoader::networkCacheCleared); statusBar = new CardPictureLoaderStatusBar(nullptr); QMainWindow *mainWindow = qobject_cast(QApplication::activeWindow()); @@ -58,7 +60,18 @@ CardPictureLoader::CardPictureLoader() : QObject(nullptr) CardPictureLoader::~CardPictureLoader() { - worker->deleteLater(); + if (worker) { + // Capture the thread first: shutdownThread() blocks until the worker has been freed by the + // finished() -> deleteLater chain, after which the worker pointer must not be dereferenced. + QThread *pictureLoaderThread = worker->workerThread(); + const bool stopped = worker->shutdownThread(); + worker = nullptr; + // Deleting a QThread that is still running is undefined behaviour, so only free it once the + // bounded wait in shutdownThread() confirmed that it stopped. + if (stopped) { + delete pictureLoaderThread; + } + } } void CardPictureLoader::getCardBackPixmap(QPixmap &pixmap, QSize size) @@ -457,7 +470,17 @@ void CardPictureLoader::clearPixmapCache() void CardPictureLoader::clearNetworkCache() { - getInstance().worker->clearNetworkCache(); + // During teardown the worker is released before this singleton, so a queued clear may still + // arrive with no worker left to run it. + CardPictureLoaderWorker *worker = getInstance().worker; + if (!worker) { + return; + } + // The disk cache and redirect cache are owned by the worker thread, so the clear has to run + // there. Invoke it asynchronously to keep the GUI responsive while the worker may be walking + // the user's picture directories or recursively deleting the cache directory; callers that + // need to know when it is done can listen for networkCacheCleared(). + QMetaObject::invokeMethod(worker, &CardPictureLoaderWorker::clearNetworkCache, Qt::QueuedConnection); } void CardPictureLoader::cacheCardPixmaps(const QList &cards) diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h index 0a4934e6d..6224befbf 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader.h +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader.h @@ -117,6 +117,9 @@ public: public slots: /** * @brief Clears the network disk cache of the worker. + * + * The clear runs on the worker thread, so this returns before it has completed; connect to + * networkCacheCleared() to act once it is done. */ static void clearNetworkCache(); @@ -131,6 +134,10 @@ public slots: void installPrintingOverride(const ExactCard &originalCard, const ExactCard &overrideCard); void installPrintingOverrideOnLoad(const ExactCard &originalCard, const ExactCard &overrideCard); +signals: + /** @brief Emitted after the worker has finished clearing the network and redirect caches. */ + void networkCacheCleared(); + private slots: /** * @brief Triggered when the user changes the picture download settings. diff --git a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp b/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp index c82fca403..12165e8f3 100644 --- a/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp +++ b/cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp @@ -101,7 +101,7 @@ QImage CardPictureLoaderLocal::tryLoadCardImageFromDisk(const QString &setName, for (const QString &path : candidatePaths) { QFileInfo fileInfo(path); QDir dir = fileInfo.dir(); - QString baseName = fileInfo.fileName(); + QString baseName = fileInfo.completeBaseName(); if (!dir.exists()) { continue; 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..0fd6a42d5 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 @@ -16,13 +16,16 @@ #include #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 MAX_REQUESTS_PER_SEC = DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT; +static constexpr int MIN_HOST_QUOTA = DownloadSettings::MIN_HOST_REQUEST_LIMIT; +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 +static constexpr int THREAD_SHUTDOWN_WAIT_MS = 5000; ///< Bounded wait for the worker thread to stop at exit CardPictureLoaderWorker::CardPictureLoaderWorker() : QObject(nullptr), picDownload(SettingsCache::instance().downloads().getPicDownload()), - requestQuota(MAX_REQUESTS_PER_SEC) + hostRequestLimits(SettingsCache::instance().downloads().getHostRequestLimits()) { networkManager = new QNetworkAccessManager(this); // We need a timeout to ensure requests don't hang indefinitely in case of @@ -57,20 +60,59 @@ CardPictureLoaderWorker::CardPictureLoaderWorker() localLoader = new CardPictureLoaderLocal(this); pictureLoaderThread = new QThread; + // The worker object frees itself once its thread finishes, so no event loop is left + // running and the QThread is never destroyed while still executing. + connect(pictureLoaderThread, &QThread::finished, this, &QObject::deleteLater); 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); + + connect(&SettingsCache::instance().downloads(), &DownloadSettings::hostRequestLimitsChanged, this, + [this] { hostRequestLimits = SettingsCache::instance().downloads().getHostRequestLimits(); }); } CardPictureLoaderWorker::~CardPictureLoaderWorker() { saveRedirectCache(); - pictureLoaderThread->deleteLater(); +} + +bool CardPictureLoaderWorker::shutdownThread() +{ + // The finished() -> deleteLater chain (wired in the constructor) frees this worker as soon as + // its event loop exits, so nothing - not even a member read - may run once wait() returns. + // QThread::quit() and QThread::wait() are thread-safe and may be called from the owning thread. + QThread *thread = pictureLoaderThread; + if (!thread) { + return true; + } + thread->quit(); + // Only an unbounded wait() would guarantee the thread stops, but this runs from a function-local + // static destructor after main() has returned, with no UI left to interrupt a worker stuck in a + // slow slot or on a stalled filesystem. Bound the wait and leave such a thread to the OS rather + // than hanging the process forever. + if (!thread->wait(THREAD_SHUTDOWN_WAIT_MS)) { + qCWarning(CardPictureLoaderWorkerLog) << "Picture loader worker thread did not stop within" + << THREAD_SHUTDOWN_WAIT_MS << "ms; leaving it to be torn down by the OS"; + return false; + } + return true; +} + +QThread *CardPictureLoaderWorker::workerThread() const +{ + return pictureLoaderThread; } void CardPictureLoaderWorker::queueRequest(const QUrl &url, CardPictureLoaderWorkerWork *worker) @@ -99,7 +141,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); } @@ -110,67 +162,258 @@ 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); QNetworkReply *reply = networkManager->get(req); - // Connect reply handling - connect(reply, &QNetworkReply::finished, worker, [reply, worker] { worker->handleNetworkReply(reply); }); + // Track in-flight replies per host so the unlocked fast path can bound how many requests it + // issues at once, instead of creating replies that time out before Qt opens a connection. + const QString host = url.host(); + hostInFlight.insert(host, hostInFlight.value(host) + 1); + + // Release the in-flight slot when the reply is destroyed, not when it emits `finished`, and use + // the worker (not the work object) as the context object: a reply can go away without ever + // finishing (aborted, or a work object deleted while a reply is still pending), and a connection + // bound to that work object's lifetime would then never run, permanently shrinking the fast + // path's concurrency until it wedges. This way the slot is released exactly once. + connect(reply, &QObject::destroyed, this, + [this, host] { hostInFlight.insert(host, qMax(0, hostInFlight.value(host) - 1)); }); + + // Connect reply handling; the work object is the context so its handler dies with it. + connect(reply, &QNetworkReply::finished, worker, [worker, reply] { worker->handleNetworkReply(reply); }); return reply; } void CardPictureLoaderWorker::resetRequestQuota() { - requestQuota = MAX_REQUESTS_PER_SEC; - QDateTime now = QDateTime::currentDateTime(); - for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end(); ++it) { + for (auto it = hostRequestQuota.begin(); it != hostRequestQuota.end();) { if (!hostLast429.contains(it.key()) || now.msecsTo(hostLast429.value(it.key())) < -QUOTA_RECOVER_MS) { - it.value() = qMin(MAX_REQUESTS_PER_SEC, it.value() + 1); + if (hostAllowanceCeiling(it.key()) == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A developer-unlocked host that fell back after a 429 recovers towards the default + // allowance; once it gets there it becomes unlocked (fast-path) again. + if (it.value() + 1 >= DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT) { + it = hostRequestQuota.erase(it); + continue; + } + it.value() += 1; + } else { + // Recover towards the host's effective allowance ceiling, which may be + // lowered by the user's per-host request limits. + it.value() = qMin(hostAllowanceCeiling(it.key()), it.value() + 1); + } } + ++it; } - 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(); - 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; + } + + QDateTime now = QDateTime::currentDateTime(); + bool dispatched = false; + // Set while an unlocked host still has queued work blocked only by the in-flight cap; the + // timer must keep running so it gets another try as soon as a slot frees. A host blocked by + // its 429 backoff instead waits for the next quota-reset tick to restart the dispatcher. + bool unlockedCapped = false; + + // Unlocked hosts (developer cap UNLIMITED_HOST_QUOTA) skip the pacing and the per-host + // allowance: dispatch their queued requests back-to-back, bounded by their 429 backoff and the + // per-host in-flight cap so a large burst can't queue replies that time out before Qt opens a + // connection for them. + for (int i = 0; i < requestLoadQueue.size();) { + const auto &request = requestLoadQueue.at(i); + // Dispatch decisions must key on the host the request will actually go to, not the URL that + // merely redirects to it: a redirect learned after this URL was queued would otherwise + // bypass the in-flight cap and drain the whole queue onto the target host unchecked. + const QUrl resolvedUrl = resolveCachedRedirect(request.first); + const QString host = resolvedUrl.host(); + if (isUnlockedHost(host)) { + if (CardPictureLoaderWorkerWork::rateLimiter().isRateLimited(host, now)) { + ++i; + continue; + } + if (hostInFlight.value(host) < MAX_IN_FLIGHT_PER_HOST) { + makeRequest(resolvedUrl, request.second); + requestLoadQueue.removeAt(i); + dispatched = true; + continue; + } + unlockedCapped = true; + } + ++i; + } + + if (requestLoadQueue.isEmpty()) { + dispatchTimer.stop(); + requestTimer.stop(); + return; + } + + if (processSingleRequest()) { + dispatched = true; + } + + // Keep the timer running while there is progress to make or unlocked work waiting on a free + // in-flight slot; otherwise no host has allowance left this second, so wait for the quota reset. + if (!dispatched && !unlockedCapped) { + 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(); } } 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); + // Copy the entry: takeAt(i) below erases within the list this reference points into. + const auto request = requestLoadQueue.at(i); + // Resolve cached redirects so the rate-limit and allowance arithmetic keys on the host the + // request will actually hit (see resolveCachedRedirect). + const QUrl resolvedUrl = resolveCachedRedirect(request.first); + const QString host = resolvedUrl.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. + auto entry = requestLoadQueue.takeAt(i); + if (host != entry.first.host()) { + // A cached redirect target is what is blocked, which the work object would not + // discover from its own URL; wait out that specific host (with jitter) instead. + entry.second->scheduleDeferredRetry(host); + } else { + entry.second->startNextPicDownload(); + } + return true; + } + // Unlocked hosts are handled by dispatchQueuedRequest's fast path, bounded by the in-flight + // cap; they must not fall through to the per-host allowance arithmetic below. + if (isUnlockedHost(host)) { + continue; + } + int ceiling = hostAllowanceCeiling(host); + if (ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA) { + // A 429 dropped this unlocked host out of the fast path and installed a concrete + // allowance; pace it against that allowance until the recovery loop unlocks it again. + ceiling = hostRequestQuota.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + } + // Seed the allowance lazily so a host that enters the queue mid-second gets its reduced + // per-host allowance, clamped against the ceiling so a lowered user cap applies from this + // second onward. + if (!hostQuotaRemaining.contains(host)) { + hostQuotaRemaining.insert(host, qMin(ceiling, hostRequestQuota.value(host, ceiling))); + } + 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); + makeRequest(resolvedUrl, entry.second); 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; +} + +int CardPictureLoaderWorker::hostAllowanceCeiling(const QString &host) const +{ + const int devCap = DownloadSettings::getDeveloperHostCaps().value(host, MAX_REQUESTS_PER_SEC); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestLimits.contains(host)) { + return DownloadSettings::UNLIMITED_HOST_QUOTA; + } + const int requested = hostRequestLimits.value(host, devCap); + return SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); +} + +bool CardPictureLoaderWorker::isUnlockedHost(const QString &host) const +{ + return hostAllowanceCeiling(host) == DownloadSettings::UNLIMITED_HOST_QUOTA && !hostRequestQuota.contains(host); +} + void CardPictureLoaderWorker::onHostRateLimited(const QString &host) { - hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, MAX_REQUESTS_PER_SEC) / 2)); + const int ceiling = hostAllowanceCeiling(host); + // An unlocked host has no per-host allowance to halve. Install one instead so it drops out of + // the unlocked fast path and is paced like a throttled host; the recovery loop in + // resetRequestQuota() then walks it back up and unlocks it again. + const int base = + ceiling == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : ceiling; + hostRequestQuota.insert(host, qMax(MIN_HOST_QUOTA, hostRequestQuota.value(host, base) / 2)); hostLast429.insert(host, QDateTime::currentDateTime()); } @@ -228,6 +471,22 @@ QUrl CardPictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const return {}; } +QUrl CardPictureLoaderWorker::resolveCachedRedirect(const QUrl &url) const +{ + // Follow the whole cached-redirect chain so dispatch keys on the host that is really hit. The + // depth bound keeps a corrupt or self-referencing cache entry from spinning us forever. + QUrl resolved = url; + int depth = 0; + while (depth++ < MAX_REDIRECT_CHAIN_DEPTH) { + QUrl target = getCachedRedirect(resolved); + if (target.isEmpty() || target == resolved) { + break; + } + resolved = target; + } + return resolved; +} + void CardPictureLoaderWorker::loadRedirectCache() { QSettings settings(cacheFilePath, QSettings::IniFormat); @@ -280,4 +539,5 @@ void CardPictureLoaderWorker::clearNetworkCache() { networkManager->cache()->clear(); redirectCache.clear(); + emit networkCacheCleared(); } 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..8b136f309 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 @@ -74,10 +74,37 @@ public: */ void onHostRateLimited(const QString &host); - /** @brief Clears the network cache and redirect cache. */ - void clearNetworkCache(); + /** + * @brief Stops the worker thread and reports whether it stopped. + * + * Called from the owning thread (CardPictureLoader) on its way out. QThread::quit() posts an + * exit request to the worker's event loop and QThread::wait() blocks (bounded) until the loop + * has returned and the thread finished. Only QThread members are touched here, so this method + * is safe to call from the owning thread. The worker object itself is freed by the finished() + * -> deleteLater chain (see the constructor); the QThread object is deleted afterwards by the + * owner (CardPictureLoader::~CardPictureLoader), not by this method. + * + * @return true if the thread stopped within the timeout, false if it is still running (in + * which case the owner must not delete the QThread). + */ + [[nodiscard]] bool shutdownThread(); + + /** + * @brief Returns the worker's QThread. + * @return The worker thread + * + * Only meaningful while the worker object is alive; capture it before calling shutdownThread(). + */ + QThread *workerThread() const; public slots: + /** + * @brief Clears the network cache and redirect cache. + * + * Runs on the worker thread; invoke it via a queued call when coming from another thread, + * since both caches are owned by the worker thread. + */ + void clearNetworkCache(); /** * @brief Makes a network request for the given URL using the specified worker. * @param url URL to load @@ -86,9 +113,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,18 +148,56 @@ 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 + QHash hostRequestLimits; ///< User-set per-host request allowances QHash hostQuotaRemaining; ///< Per-host allowance left in the current second QHash hostLast429; ///< When each host was last rate limited + QHash hostInFlight; ///< Network replies currently in flight, per host + + /** @brief Maximum concurrent in-flight network replies per host. */ + static constexpr int MAX_IN_FLIGHT_PER_HOST = 6; + + /** @brief Bound on how many cached-redirect hops dispatch resolution will follow. */ + static constexpr int MAX_REDIRECT_CHAIN_DEPTH = 10; CardPictureLoaderLocal *localLoader; ///< Loader for local images QSet currentlyLoading; ///< Deduplication: contains pixmapCacheKey currently being loaded + /** + * @brief Effective per-host allowance ceiling for a host. + * @param host The host to look up + * @return The allowance ceiling in requests/second, or DownloadSettings::UNLIMITED_HOST_QUOTA + * when the developer unlocked the host and no user limit is set for it. + */ + [[nodiscard]] int hostAllowanceCeiling(const QString &host) const; + + /** + * @brief Whether a host may skip dispatch pacing and per-host allowance entirely. + * + * A host is unlocked while it has no user limit and no reduced allowance installed by a 429. + * A 429 drops it out of the fast path until resetRequestQuota() walks the allowance back up. + */ + [[nodiscard]] bool isUnlockedHost(const QString &host) const; + /** @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 Follows the cached-redirect chain to the URL that will actually be requested. + * @param url The URL to resolve + * @return The final URL after chasing cached redirects, or @p url itself if none lead elsewhere + * + * Dispatch decisions (unlocked-host fast path, 429 backoff, in-flight cap) must key on the host + * a request really goes to, not the URL that merely redirects to it. + */ + [[nodiscard]] QUrl resolveCachedRedirect(const QUrl &url) const; + /** @brief Loads redirect cache from disk. */ void loadRedirectCache(); @@ -139,6 +207,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(); @@ -158,6 +229,9 @@ signals: /** @brief Emitted when a network request successfully completes. */ void imageRequestSucceeded(const QUrl &url); + + /** @brief Emitted after clearNetworkCache() has finished clearing both caches. */ + void networkCacheCleared(); }; #endif // PICTURE_LOADER_WORKER_H 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..19c6be469 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,8 +22,13 @@ static const QStringList MD5_BLACKLIST = { "fbc7d763c08771c260b39e2115414eeb" // Current card back hash }; -CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad) - : QObject(nullptr), cardToDownload(CardPictureToLoad(toLoad)), +const ServerRateLimiter &CardPictureLoaderWorkerWork::rateLimiter() +{ + return s_rateLimiter; +} + +CardPictureLoaderWorkerWork::CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad) + : QObject(worker), cardToDownload(CardPictureToLoad(toLoad)), picDownload(SettingsCache::instance().downloads().getPicDownload()) { // Hook up signals to the orchestrator @@ -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..84a7dafdb 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 @@ -36,13 +37,36 @@ class CardPictureLoaderWorkerWork : public QObject public: /** * @brief Constructs a worker for downloading a specific card image. - * @param worker The orchestrating CardPictureLoaderWorker + * @param worker The orchestrating CardPictureLoaderWorker; the work object becomes its child so + * it is destroyed with the worker even if it never reaches concludeImageLoad(). * @param toLoad The ExactCard to download */ - explicit CardPictureLoaderWorkerWork(const CardPictureLoaderWorker *worker, const ExactCard &toLoad); + explicit CardPictureLoaderWorkerWork(CardPictureLoaderWorker *worker, const ExactCard &toLoad); 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 +79,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 +103,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(); diff --git a/cockatrice/src/interface/theme_manager.cpp b/cockatrice/src/interface/theme_manager.cpp index d86ed77f9..d93dfcf62 100644 --- a/cockatrice/src/interface/theme_manager.cpp +++ b/cockatrice/src/interface/theme_manager.cpp @@ -95,9 +95,11 @@ struct PaletteColorInfo static QString usableDefaultStyle(const QString &style) { - // The Windows 11 native style is broken: when the OS default - // ("System" theme selection) would use it, fall back to the Vista style. - // Explicitly choosing "windows11" in a theme is still honored. + // The Windows 11 native style is broken: dragging cards across zones can + // shrink the board to a tiny grey window that is unfixable without + // rejoining. It is never usable, so guard against it no matter how it was + // requested (OS default or an explicit "windows11" theme choice) and fall + // back to the Vista style. return style.compare("windows11", Qt::CaseInsensitive) == 0 ? QStringLiteral("windowsvista") : style; } @@ -109,7 +111,10 @@ ThemeManager::ThemeManager(QObject *parent) : QObject(parent) ensureThemeDirectoryExists(); #if (QT_VERSION >= QT_VERSION_CHECK(6, 5, 0)) connect(QGuiApplication::styleHints(), &QStyleHints::colorSchemeChanged, this, [this] { - defaultPalette = qApp->palette(); + // Reload so scheme-qualified assets and palettes follow the OS, but do + // NOT recapture defaultPalette: qApp->palette() already carries the + // currently-applied theme palette at this point, so recapturing it + // would contaminate the base for every later theme switch. themeChangedSlot(); }); #endif @@ -407,6 +412,10 @@ void ThemeManager::applyStyleAndPalette(const QString &themeName, } } + // The Windows 11 style is broken even when selected explicitly in a theme, + // so sanitize the resolved name here rather than trusting the theme config. + styleName = usableDefaultStyle(styleName); + QStyle *style = QStyleFactory::create(styleName); if (!style) { style = QStyleFactory::create(usableDefaultStyle(defaultStyleName)); diff --git a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp index f161f0f19..415eef6b9 100644 --- a/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/appearance_settings_page.cpp @@ -62,6 +62,12 @@ AppearanceSettingsPage::AppearanceSettingsPage() // Qt widget style; "System" lets the application decide styleCombo.addItem(tr("System"), QStringLiteral("System")); for (const QString &key : QStyleFactory::keys()) { + // The Windows 11 native style is broken (board rendering glitches when + // moving cards), so never offer it; it is already sanitized at apply + // time in ThemeManager. + if (key.compare("windows11", Qt::CaseInsensitive) == 0) { + continue; + } styleCombo.addItem(key, key); } @@ -505,7 +511,7 @@ void AppearanceSettingsPage::retranslateUi() openThemeButton.setText(tr("Open themes folder")); schemeComboLabel.setText(tr("Active theme palette:")); styleComboLabel.setText(tr("Active theme style:")); - styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"Default\" lets the application decide)")); + styleCombo.setToolTip(tr("Qt widget style saved to this theme (\"System\" lets the application decide)")); editPaletteButton.setText(tr("Edit theme palette")); homeTabGroupBox->setTitle(tr("Home tab settings")); diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp index f3eac05b8..bdbb38c95 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.cpp @@ -10,7 +10,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -51,7 +53,9 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() urlList->setDragDropMode(QAbstractItemView::InternalMove); connect(urlList->model(), &QAbstractItemModel::rowsMoved, this, &DeckEditorSettingsPage::urlListChanged); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } aAdd = new QAction(this); aAdd->setIcon(themePixmap(QStringLiteral("icons/increment"))); @@ -65,11 +69,16 @@ DeckEditorSettingsPage::DeckEditorSettingsPage() aRemove->setIcon(themePixmap(QStringLiteral("icons/decrement"))); connect(aRemove, &QAction::triggered, this, &DeckEditorSettingsPage::actRemoveURL); + aRateLimit = new QAction(this); + aRateLimit->setIcon(themePixmap(QStringLiteral("icons/cogwheel"))); + connect(aRateLimit, &QAction::triggered, this, &DeckEditorSettingsPage::actAdjustRateLimit); + auto *urlToolBar = new QToolBar; urlToolBar->setOrientation(Qt::Vertical); urlToolBar->addAction(aAdd); urlToolBar->addAction(aRemove); urlToolBar->addAction(aEdit); + urlToolBar->addAction(aRateLimit); urlToolBar->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::MinimumExpanding); auto *urlListLayout = new QHBoxLayout; @@ -117,7 +126,9 @@ void DeckEditorSettingsPage::resetDownloadedURLsButtonClicked() { SettingsCache::instance().downloads().resetToDefaultURLs(); urlList->clear(); - urlList->addItems(SettingsCache::instance().downloads().getAllURLs()); + for (const QString &url : SettingsCache::instance().downloads().getAllURLs()) { + addUrlItem(url); + } QMessageBox::information(this, tr("Success"), tr("Download URLs have been reset.")); } @@ -126,7 +137,7 @@ void DeckEditorSettingsPage::actAddURL() bool ok; QString msg = QInputDialog::getText(this, tr("Add URL"), tr("URL:"), QLineEdit::Normal, QString(), &ok); if (ok) { - urlList->addItem(msg); + addUrlItem(msg); storeSettings(); } } @@ -141,12 +152,14 @@ void DeckEditorSettingsPage::actRemoveURL() void DeckEditorSettingsPage::actEditURL() { - if (urlList->currentItem()) { - QString oldText = urlList->currentItem()->text(); + QListWidgetItem *item = urlList->currentItem(); + if (item) { + const QString oldText = urlForItem(item); bool ok; QString msg = QInputDialog::getText(this, tr("Edit URL"), tr("URL:"), QLineEdit::Normal, oldText, &ok); if (ok) { - urlList->currentItem()->setText(msg); + item->setData(Qt::UserRole, msg); + item->setText(urlLabel(msg)); storeSettings(); } } @@ -158,10 +171,133 @@ void DeckEditorSettingsPage::storeSettings() QStringList downloadUrls; for (int i = 0; i < urlList->count(); i++) { - qInfo() << "Priority" << i << ":" << urlList->item(i)->text(); - downloadUrls << urlList->item(i)->text(); + const QString url = urlForItem(urlList->item(i)); + qInfo() << "Priority" << i << ":" << url; + downloadUrls << url; } SettingsCache::instance().downloads().setDownloadUrls(downloadUrls); + + // Drop per-host limits whose host is no longer referenced by any configured URL, so removing + // a URL doesn't leave a stale throttle behind that reactivates if the host is re-added. + QSet usedHosts; + for (const QString &url : downloadUrls) { + const QString host = QUrl(url).host(); + if (!host.isEmpty()) { + usedHosts.insert(host); + } + } + QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + bool limitsChanged = false; + for (auto it = limits.begin(); it != limits.end();) { + // Prune only limits for hosts that are neither referenced by a configured URL nor carry a + // developer cap. Capped hosts are often redirect targets (e.g. api.scryfall.com redirects + // to cards.scryfall.io) that never appear in the URL list, yet they are exactly the hosts + // the throttle applies to, so dropping them when a URL is removed would silently re-enable + // free-running traffic to a rate-sensitive server. + if (!usedHosts.contains(it.key()) && !DownloadSettings::getDeveloperHostCaps().contains(it.key())) { + it = limits.erase(it); + limitsChanged = true; + } else { + ++it; + } + } + if (limitsChanged) { + SettingsCache::instance().downloads().setHostRequestLimits(limits); + } + + refreshUrlItems(); +} + +QListWidgetItem *DeckEditorSettingsPage::addUrlItem(const QString &url) +{ + auto *item = new QListWidgetItem(urlLabel(url)); + item->setData(Qt::UserRole, url); + urlList->addItem(item); + return item; +} + +QString DeckEditorSettingsPage::urlForItem(const QListWidgetItem *item) const +{ + return item->data(Qt::UserRole).toString(); +} + +QString DeckEditorSettingsPage::urlLabel(const QString &url) const +{ + const QString host = QUrl(url).host(); + if (host.isEmpty()) { + return url; + } + + const QHash limits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = + DownloadSettings::getDeveloperHostCaps().value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == DownloadSettings::UNLIMITED_HOST_QUOTA && !limits.contains(host)) { + return tr("%1 (unlimited)").arg(url); + } + + const int requested = limits.value( + host, devCap == DownloadSettings::UNLIMITED_HOST_QUOTA ? DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT : devCap); + const int effective = SettingsCache::instance().downloads().clampHostRequestLimit(host, requested); + return tr("%1 (%2/s)").arg(url).arg(effective); +} + +void DeckEditorSettingsPage::refreshUrlItems() +{ + for (int i = 0; i < urlList->count(); ++i) { + QListWidgetItem *item = urlList->item(i); + item->setText(urlLabel(urlForItem(item))); + } +} + +void DeckEditorSettingsPage::actAdjustRateLimit() +{ + if (urlList->currentItem() == nullptr) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("Select a URL in the list first.")); + return; + } + + const QString host = QUrl(urlForItem(urlList->currentItem())).host(); + if (host.isEmpty()) { + QMessageBox::information(this, tr("Adjust Rate Limit"), tr("The selected URL does not have a valid host.")); + return; + } + + const QHash &devCaps = DownloadSettings::getDeveloperHostCaps(); + const QHash currentLimits = SettingsCache::instance().downloads().getHostRequestLimits(); + const int devCap = devCaps.value(host, DownloadSettings::DEFAULT_HOST_REQUEST_LIMIT); + const bool unlocked = devCap == DownloadSettings::UNLIMITED_HOST_QUOTA; + + bool ok = false; + int minimum; + int maximum; + int defaultValue; + QString prompt; + if (unlocked) { + minimum = 0; // 0 means "unlimited" + maximum = DownloadSettings::UNLOCKED_HOST_LIMIT_MAX; + defaultValue = currentLimits.value(host, 0); + prompt = tr("Requests per second (0 = unlimited, fastest; up to %1):").arg(maximum); + } else { + minimum = DownloadSettings::MIN_HOST_REQUEST_LIMIT; + maximum = devCap; + defaultValue = currentLimits.value(host, devCap); + prompt = tr("Requests per second (developer maximum is %1):").arg(maximum); + } + + const int value = QInputDialog::getInt(this, tr("Adjust Rate Limit for %1").arg(host), prompt, defaultValue, + minimum, maximum, 1, &ok); + if (!ok) { + return; + } + + QHash limits = currentLimits; + if (unlocked ? value == 0 : value == devCap) { + limits.remove(host); + } else { + limits.insert(host, value); + } + SettingsCache::instance().downloads().setHostRequestLimits(limits); + refreshUrlItems(); } void DeckEditorSettingsPage::urlListChanged(const QModelIndex &, int, int, const QModelIndex &, int) @@ -244,4 +380,8 @@ void DeckEditorSettingsPage::retranslateUi() aAdd->setText(tr("Add New URL")); aEdit->setText(tr("Edit URL")); aRemove->setText(tr("Remove URL")); -} \ No newline at end of file + aRateLimit->setText(tr("Adjust Rate Limit")); + + // The per-URL rate limit suffixes are translated, so refresh them when the language changes. + refreshUrlItems(); +} diff --git a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h index 5db009c8a..23e33fa20 100644 --- a/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/deck_editor_settings_page.h @@ -27,6 +27,7 @@ private slots: void actAddURL(); void actRemoveURL(); void actEditURL(); + void actAdjustRateLimit(); void resetDownloadedURLsButtonClicked(); private: @@ -34,7 +35,7 @@ private: QLabel urlLinkLabel; QCheckBox picDownloadCheckBox; QListWidget *urlList; - QAction *aAdd, *aEdit, *aRemove; + QAction *aAdd, *aEdit, *aRemove, *aRateLimit; QCheckBox mcDownloadSpoilersCheckBox; QLabel msDownloadSpoilersLabel; QGroupBox *mpGeneralGroupBox; @@ -46,6 +47,18 @@ private: QLabel infoOnSpoilersLabel; QPushButton *mpSpoilerPathButton; QPushButton *updateNowButton; + + /** @brief Adds a list item for the given URL, storing the raw URL alongside its displayed label. */ + QListWidgetItem *addUrlItem(const QString &url); + + /** @brief Returns the raw URL stored on a list item. */ + [[nodiscard]] QString urlForItem(const QListWidgetItem *item) const; + + /** @brief Returns the display label for a URL, including its current effective rate limit. */ + [[nodiscard]] QString urlLabel(const QString &url) const; + + /** @brief Refreshes the displayed label of every URL item after limits or settings change. */ + void refreshUrlItems(); }; #endif // COCKATRICE_DECK_EDITOR_SETTINGS_PAGE_H diff --git a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp index 17838e501..4aad91115 100644 --- a/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/storage_settings_page.cpp @@ -181,9 +181,14 @@ StorageSettingsPage::StorageSettingsPage() void StorageSettingsPage::clearDownloadedPicsButtonClicked() { - CardPictureLoader::clearNetworkCache(); + // The network cache is cleared asynchronously on the worker thread, so wait for the completion + // signal before confirming; the in-memory pixmap cache is cleared synchronously right away. + connect( + &CardPictureLoader::getInstance(), &CardPictureLoader::networkCacheCleared, this, + [this] { QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); }, + Qt::SingleShotConnection); CardPictureLoader::clearPixmapCache(); - QMessageBox::information(this, tr("Success"), tr("Cached card pictures have been reset.")); + CardPictureLoader::clearNetworkCache(); } void StorageSettingsPage::clearImageBackupsButtonClicked() diff --git a/cockatrice/themes/System/palette-default-dark.toml b/cockatrice/themes/System/palette-default-dark.toml index e101935b4..9dfe6e19f 100644 --- a/cockatrice/themes/System/palette-default-dark.toml +++ b/cockatrice/themes/System/palette-default-dark.toml @@ -1,67 +1,7 @@ -[Palette] -WindowText = #ffffffff -Button = #ff383838 -Light = #ff737373 -Midlight = #ff525252 -Dark = #ff161616 -Mid = #ff252525 -Text = #ffffffff -BrightText = #ffb4dd8b -ButtonText = #ffffffff -Base = #ff2b2b2b -Window = #ff1c1c1c -Shadow = #ff000000 -HighlightedText = #ff000000 -Link = #ffcde4b6 -LinkVisited = #ff99d999 -AlternateBase = #ff242424 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #6effffff - -[Palette.Disabled] -WindowText = #ff9d9d9d -Button = #ff1c1c1c -Light = #ff737373 -Midlight = #ff525252 -Dark = #ff161616 -Mid = #ff252525 -Text = #ff9d9d9d -BrightText = #ffb4dd8b -ButtonText = #ff787878 -Base = #ff1c1c1c -Window = #ff1c1c1c -Shadow = #ff000000 -HighlightedText = #ff9d9d9d -Link = #ff308cc6 -LinkVisited = #ffb450ff -AlternateBase = #ff242424 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #46ffffff - -[Palette.Inactive] -WindowText = #ffffffff -Button = #ff383838 -Light = #ff737373 -Midlight = #ff525252 -Dark = #ff161616 -Mid = #ff252525 -Text = #ffffffff -BrightText = #ffb4dd8b -ButtonText = #ffffffff -Base = #ff2b2b2b -Window = #ff1c1c1c -Shadow = #ff000000 -HighlightedText = #ffffffff -Link = #ffcde4b6 -LinkVisited = #ff99d999 -AlternateBase = #ff242424 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #6effffff - - +# The System theme derives all of its colors from the platform palette +# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette +# overrides. Only the identity app colors are provided here, so buttons keep +# their classic greens while the actual palette stays native to the OS. [AppColors] AccentStrong = #ff148c3c AccentSoft = #ff78c850 diff --git a/cockatrice/themes/System/palette-default-light.toml b/cockatrice/themes/System/palette-default-light.toml index 14c215cdf..9dfe6e19f 100644 --- a/cockatrice/themes/System/palette-default-light.toml +++ b/cockatrice/themes/System/palette-default-light.toml @@ -1,67 +1,7 @@ -[Palette] -WindowText = #ff000000 -Button = #fff0f0f0 -Light = #ffffffff -Midlight = #ffe3e3e3 -Dark = #ffa0a0a0 -Mid = #ffa0a0a0 -Text = #ff000000 -BrightText = #ffffffff -ButtonText = #ff000000 -Base = #ffffffff -Window = #fff0f0f0 -Shadow = #ff696969 -HighlightedText = #ffffffff -Link = #ff0d5f28 -LinkVisited = #ff08401b -AlternateBase = #ffe9e7e3 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #80000000 - -[Palette.Disabled] -WindowText = #ff787878 -Button = #fff0f0f0 -Light = #ffffffff -Midlight = #fff7f7f7 -Dark = #ffa0a0a0 -Mid = #ffa0a0a0 -Text = #ff787878 -BrightText = #ffffffff -ButtonText = #ff787878 -Base = #fff0f0f0 -Window = #fff0f0f0 -Shadow = #ff000000 -HighlightedText = #ffffffff -Link = #ff0000ff -LinkVisited = #ffff00ff -AlternateBase = #fff7f7f7 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #80000000 - -[Palette.Inactive] -WindowText = #ff000000 -Button = #fff0f0f0 -Light = #ffffffff -Midlight = #ffe3e3e3 -Dark = #ffa0a0a0 -Mid = #ffa0a0a0 -Text = #ff000000 -BrightText = #ffffffff -ButtonText = #ff000000 -Base = #ffffffff -Window = #fff0f0f0 -Shadow = #ff696969 -HighlightedText = #ff000000 -Link = #ff0d5f28 -LinkVisited = #ff08401b -AlternateBase = #ffe9e7e3 -ToolTipBase = #ffffffdc -ToolTipText = #ff000000 -PlaceholderText = #80000000 - - +# The System theme derives all of its colors from the platform palette +# (e.g. qt6ct's custom color scheme on Linux), so it must NOT ship palette +# overrides. Only the identity app colors are provided here, so buttons keep +# their classic greens while the actual palette stays native to the OS. [AppColors] AccentStrong = #ff148c3c -AccentSoft = #ff78c850 \ No newline at end of file +AccentSoft = #ff78c850 diff --git a/cockatrice/themes/System/theme.cfg b/cockatrice/themes/System/theme.cfg index 4b756a5e8..23fffccbb 100644 --- a/cockatrice/themes/System/theme.cfg +++ b/cockatrice/themes/System/theme.cfg @@ -1,5 +1,5 @@ [Appearance] -ColorScheme = Light +ColorScheme = System [Style] Name = System diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp index eb73e58ee..6e22a84f4 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.cpp @@ -12,6 +12,22 @@ const QStringList DownloadSettings::DEFAULT_DOWNLOAD_URLS = { const QString DownloadSettings::SCRYFALL_NAMED_LOCALIZED_URL = "https://api.scryfall.com/cards/named?fuzzy=!localizedName!&lang=!sflang!&format=image&face=!prop:side!"; +// Developer-set ceilings for the per-host request allowance. Users may lower a host's +// allowance via the download settings, but can never raise it above these values. Hosts +// not listed default to DEFAULT_HOST_REQUEST_LIMIT. A cap of UNLIMITED_HOST_QUOTA marks a +// host that is never throttled per host and skips the dispatch pacing (429 backoff still applies). +const QHash DownloadSettings::DEVELOPER_HOST_CAPS = { + // The Scryfall API enforces 10 requests/second; stay one under so a burst can't trip 429s. + {"api.scryfall.com", 9}, + // The Scryfall image CDN has no documented per-client rate limit. + {"cards.scryfall.io", UNLIMITED_HOST_QUOTA}, +}; + +const QHash &DownloadSettings::getDeveloperHostCaps() +{ + return DEVELOPER_HOST_CAPS; +} + DownloadSettings::DownloadSettings(const QString &settingPath, QObject *parent = nullptr) : SettingsManager(settingPath + "downloads.ini", "downloads", QString(), parent) { @@ -65,3 +81,57 @@ void DownloadSettings::setDownloadSpoilerStatus(bool _spoilerStatus) setValue(_spoilerStatus, "downloadSpoilers"); emit downloadSpoilerStatusChanged(); } + +QHash DownloadSettings::getHostRequestLimits() const +{ + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); + } + settings.beginGroup("hostRequestLimits"); + + QHash hostRequestLimits; + const QStringList hosts = settings.childKeys(); + for (const QString &host : hosts) { + hostRequestLimits.insert(host, settings.value(host).toInt()); + } + + settings.endGroup(); + if (!defaultGroup.isEmpty()) { + settings.endGroup(); + } + return hostRequestLimits; +} + +void DownloadSettings::setHostRequestLimits(const QHash &hostRequestLimits) +{ + auto settings = getSettings(); + if (!defaultGroup.isEmpty()) { + settings.beginGroup(defaultGroup); + } + + // Drop the legacy single-key form (an opaque @Variant blob) written by earlier builds so each + // host is stored as a plain, hand-editable key in its own subgroup. + settings.remove("hostRequestLimits"); + settings.beginGroup("hostRequestLimits"); + settings.remove(QString()); + for (auto it = hostRequestLimits.cbegin(); it != hostRequestLimits.cend(); ++it) { + settings.setValue(it.key(), it.value()); + } + settings.endGroup(); + + if (!defaultGroup.isEmpty()) { + settings.endGroup(); + } + settings.sync(); + emit hostRequestLimitsChanged(); +} + +int DownloadSettings::clampHostRequestLimit(const QString &host, int requested) const +{ + const int devCap = DEVELOPER_HOST_CAPS.value(host, DEFAULT_HOST_REQUEST_LIMIT); + if (devCap == UNLIMITED_HOST_QUOTA) { + return qMax(MIN_HOST_REQUEST_LIMIT, requested); + } + return qBound(MIN_HOST_REQUEST_LIMIT, requested, devCap); +} diff --git a/libcockatrice_settings/libcockatrice/settings/download_settings.h b/libcockatrice_settings/libcockatrice/settings/download_settings.h index ae49884f0..7eca77df6 100644 --- a/libcockatrice_settings/libcockatrice/settings/download_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/download_settings.h @@ -9,6 +9,8 @@ #include "settings_manager.h" +#include + class DownloadSettings : public SettingsManager { Q_OBJECT @@ -16,8 +18,35 @@ class DownloadSettings : public SettingsManager static const QStringList DEFAULT_DOWNLOAD_URLS; static const QString SCRYFALL_NAMED_LOCALIZED_URL; + static const QHash DEVELOPER_HOST_CAPS; public: + /** @brief Per-host request allowance (requests/second) when no developer cap applies. */ + static constexpr int DEFAULT_HOST_REQUEST_LIMIT = 10; + /** @brief Floor for any per-host request allowance. */ + static constexpr int MIN_HOST_REQUEST_LIMIT = 1; + /** + * @brief Upper bound offered to the user when lowering an unlocked host's allowance. + * + * Unlocked hosts have no developer cap, so `clampHostRequestLimit` puts no upper bound on + * them; this only bounds what the settings dialog offers, and matches the widest paced + * allowance a user is documented to be able to hand-edit in `downloads.ini`. Choosing 0 + * below this restores the "unlimited" fast path. + */ + static constexpr int UNLOCKED_HOST_LIMIT_MAX = 50; + /** @brief Developer cap marking a host as never throttled per host or by the dispatch pacing. */ + static constexpr int UNLIMITED_HOST_QUOTA = -1; + + /** + * @brief Developer-set per-host allowance ceilings (requests/second), keyed by host. + * + * Hosts not present default to DEFAULT_HOST_REQUEST_LIMIT. An entry of + * UNLIMITED_HOST_QUOTA marks a host that users may still lower, but that is never + * throttled per host by default. Users can never raise a host's allowance above its + * developer cap. + */ + static const QHash &getDeveloperHostCaps(); + explicit DownloadSettings(const QString &, QObject *); QStringList getAllURLs() const; @@ -29,9 +58,23 @@ public: [[nodiscard]] bool getDownloadSpoilersStatus() const; void setDownloadSpoilerStatus(bool _spoilerStatus); + /** @brief User-set per-host request allowances (requests/second). Missing hosts use the developer default. */ + QHash getHostRequestLimits() const; + void setHostRequestLimits(const QHash &hostRequestLimits); + + /** + * @brief Clamps the user's requested allowance for a host against its developer cap. + * @param host The host to clamp for + * @param requested The user-requested allowance in requests/second + * @return The effective allowance. Users may lower a host's allowance but never raise it + * above the developer cap; hosts with UNLIMITED_HOST_QUOTA have no upper bound. + */ + [[nodiscard]] int clampHostRequestLimit(const QString &host, int requested) const; + signals: void picDownloadChanged(); void downloadSpoilerStatusChanged(); + void hostRequestLimitsChanged(); }; #endif // COCKATRICE_DOWNLOADSETTINGS_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7bb834d7e..d23d9ba04 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -155,3 +155,9 @@ add_subdirectory(loading_from_clipboard) add_subdirectory(movecard_tests) add_subdirectory(oracle) add_subdirectory(settings) + +# picture_loader_benchmark links libcockatrice_settings, which only exists when +# a client/UI-capable target is being built. +if(WITH_ORACLE OR WITH_CLIENT) + add_subdirectory(picture_loader_benchmark) +endif() diff --git a/tests/loader_local_matching_test.cpp b/tests/loader_local_matching_test.cpp index 7ffa7157d..6ce684fdd 100644 --- a/tests/loader_local_matching_test.cpp +++ b/tests/loader_local_matching_test.cpp @@ -68,6 +68,18 @@ protected: loader = nullptr; } + /** + * @brief Destroys and rebuilds the loader so the CUSTOM index is re-scanned. + * + * The CUSTOM-folder index is snapshotted at construction; rebuild it after + * writing files so freshly placed images are discoverable. + */ + void rebuildLoader() + { + delete loader; + loader = new CardPictureLoaderLocal(nullptr); + } + /** * @brief Writes a valid 1x1 PNG under the sandboxed pics path. */ @@ -162,6 +174,28 @@ TEST_F(LocalMatcherTest, SetFolderCandidateTakesPrecedenceOverRootFallback) EXPECT_EQ(image.pixelColor(0, 0), QColor(Qt::red)) << "The set-folder candidate must be preferred"; } +TEST_F(LocalMatcherTest, CustomSubfolderResolvesExactFile) +{ + // Images in the CUSTOM folder are indexed by their full file path (extension included), + // unlike the extension-less candidate paths built from the naming schemes. + writePngUnderPics("CUSTOM/poker/TestCard.png"); + rebuildLoader(); + + const QImage image = loader->tryLoad(cardFor("TestCard", "", "")); + + EXPECT_FALSE(image.isNull()) << "A CUSTOM-folder image in a subdirectory must resolve"; +} + +TEST_F(LocalMatcherTest, CustomSubfolderIgnoresSuffixedFiles) +{ + writePngUnderPics("CUSTOM/poker/TestCard (1).png"); + rebuildLoader(); + + const QImage image = loader->tryLoad(cardFor("TestCard", "", "")); + + EXPECT_TRUE(image.isNull()) << "A suffixed CUSTOM-folder file must not satisfy an exact name lookup"; +} + } // namespace int main(int argc, char **argv) diff --git a/tests/picture_loader_benchmark/CMakeLists.txt b/tests/picture_loader_benchmark/CMakeLists.txt new file mode 100644 index 000000000..a3b75fe88 --- /dev/null +++ b/tests/picture_loader_benchmark/CMakeLists.txt @@ -0,0 +1,26 @@ +# Manual benchmark hitting the real card image hosts. Not registered with +# add_test(): it requires a cards.xml, touches the network for minutes at a +# time, and needs network access. Build it explicitly and run by hand. +add_executable( + picture_loader_benchmark_test + ${VERSION_STRING_CPP} + ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_local.cpp + ../../cockatrice/src/interface/card_picture_loader/card_picture_to_load.cpp + ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker.cpp + ../../cockatrice/src/interface/card_picture_loader/card_picture_loader_worker_work.cpp + ../../cockatrice/src/client/settings/cache_settings.h + picture_loader_benchmark.cpp + settings_cache_mock.cpp +) + +target_include_directories(picture_loader_benchmark_test PRIVATE ${CMAKE_SOURCE_DIR}/cockatrice/src) + +target_link_libraries( + picture_loader_benchmark_test + libcockatrice_card + libcockatrice_settings + libcockatrice_interfaces + libcockatrice_utility + Threads::Threads + ${TEST_QT_MODULES} +) diff --git a/tests/picture_loader_benchmark/picture_loader_benchmark.cpp b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp new file mode 100644 index 000000000..f991f2ec0 --- /dev/null +++ b/tests/picture_loader_benchmark/picture_loader_benchmark.cpp @@ -0,0 +1,547 @@ +/* + * Picture loader benchmark / regression suite against the real card image hosts. + * + * Deliberately not registered with ctest: it hits live Scryfall / Gatherer + * endpoints at ~10 requests per second and takes minutes. Run it by hand. + * + * picture_loader_benchmark_test --carddb /path/to/cards.xml [options] + * + * Modes + * ----- + * default : for every URL template in the configured download list (or for each + * --url given), load #count pictures twice: once cold (network) and + * once cached (served from the QNetworkDiskCache). Both passes must + * load every card with zero failures. The cached pass must complete + * well under the cold time, which is the regression gate for serving + * cached pictures instead of re-fetching them. The cold pass must stay + * above a pacing lower bound, the regression gate for burst-free + * request throttling. + * --stress: two CardPictureLoaderWorker instances loading the same cards + * concurrently against one host (~20 req/s aggregate), which forces + * real 429 responses. Both workers must still complete 100% of their + * cards via the shared backoff logic. + */ + +#include "client/settings/cache_settings.h" +#include "interface/card_picture_loader/card_picture_loader_worker.h" +#include "interface/card_picture_loader/card_picture_to_load.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +class BenchmarkCardDatabasePathProvider : public ICardDatabasePathProvider +{ +public: + BenchmarkCardDatabasePathProvider(QString _cardsXml, QString _customSetsDir) + : cardsXml(std::move(_cardsXml)), customSetsDir(std::move(_customSetsDir)) + { + } + + QString getCardDatabasePath() const override + { + return cardsXml; + } + + QString getCustomCardDatabasePath() const override + { + return customSetsDir; + } + + QString getTokenDatabasePath() const override + { + return QString(); + } + + QString getSpoilerCardDatabasePath() const override + { + return QString(); + } + +private: + QString cardsXml; + QString customSetsDir; +}; + +struct PassResult +{ + int enqueued = 0; + int finished = 0; + int failed = 0; + qint64 elapsedMs = 0; + QStringList failedCards; +}; + +static QList selectCardsForTemplate(CardDatabase &db, const QString &urlTemplate, int maxCards) +{ + QList selected; + const QList cards = db.getCardList().values(); + for (const CardInfoPtr &card : cards) { + if (selected.size() >= maxCards) { + break; + } + const SetToPrintingsMap &sets = card->getSets(); + if (sets.isEmpty()) { + continue; + } + const QList printings = sets.first(); + if (printings.isEmpty()) { + continue; + } + const ExactCard cardToLoad(card, printings.first()); + if (CardPictureToLoad(cardToLoad).transformUrl(urlTemplate).isEmpty()) { + continue; + } + selected.append(cardToLoad); + } + return selected; +} + +static PassResult runPass(CardPictureLoaderWorker *worker, const QList &cards, int timeoutMs) +{ + PassResult result; + result.enqueued = cards.size(); + + QEventLoop loop; + QTimer watchdog; + watchdog.setSingleShot(true); + watchdog.setInterval(timeoutMs); + QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit); + + QElapsedTimer clock; + QObject::connect(worker, &CardPictureLoaderWorker::imageLoaded, &loop, + [&](const ExactCard &card, const QImage &image) { + ++result.finished; + if (image.isNull()) { + ++result.failed; + if (result.failedCards.size() < 10) { + result.failedCards.append(card.getName()); + } + } + if (result.finished >= result.enqueued) { + loop.quit(); + } + }); + + clock.start(); + for (const ExactCard &card : cards) { + worker->enqueueImageLoad(card); + } + watchdog.start(); + loop.exec(); + result.elapsedMs = clock.elapsed(); + return result; +} + +struct StressResult +{ + PassResult a; + PassResult b; + int http429Count = 0; +}; + +struct LogCounters +{ + int http429 = 0; + QMutex mutex; +}; + +static std::atomic s_activeCounters{nullptr}; +static QtMessageHandler s_previousMessageHandler = nullptr; + +static void stressLogHandler(QtMsgType type, const QMessageLogContext &context, const QString &msg) +{ + if (LogCounters *counters = s_activeCounters.load(std::memory_order_acquire); + counters && msg.contains(QStringLiteral("Too many requests from"))) { + QMutexLocker locker(&counters->mutex); + ++counters->http429; + } + if (s_previousMessageHandler) { + s_previousMessageHandler(type, context, msg); + } else { + // qInstallMessageHandler() reports the built-in handler as nullptr, so a plain `if` would + // swallow every message - including the 429 warnings this run is meant to surface. Fall + // back to Qt's message pattern written to stderr instead. + std::fprintf(stderr, "%s\n", qPrintable(qFormatLogMessage(type, context, msg))); + } +} + +// Stops a worker's thread and frees it. shutdownThread()'s bounded wait guarantees that the worker +// object was freed by its finished() -> deleteLater chain, so the thread itself can then be deleted +// safely. A worker whose thread refused to stop is left alone (and leaked) rather than freed while +// still running. +static void destroyWorker(CardPictureLoaderWorker *worker) +{ + if (!worker) { + return; + } + QThread *thread = worker->workerThread(); + if (worker->shutdownThread()) { + delete thread; + } +} + +static StressResult runStress(CardPictureLoaderWorker *workerA, + CardPictureLoaderWorker *workerB, + const QList &cards, + int timeoutMs) +{ + StressResult result; + result.a.enqueued = cards.size(); + result.b.enqueued = cards.size(); + + int completed = 0; + QMutex completedMutex; + + QEventLoop loop; + QTimer watchdog; + watchdog.setSingleShot(true); + watchdog.setInterval(timeoutMs); + QObject::connect(&watchdog, &QTimer::timeout, &loop, &QEventLoop::quit); + + const auto finishOne = [&](PassResult &pass, const ExactCard &card, const QImage &image) { + ++pass.finished; + if (image.isNull()) { + ++pass.failed; + if (pass.failedCards.size() < 10) { + pass.failedCards.append(card.getName()); + } + } + QMutexLocker locker(&completedMutex); + ++completed; + if (completed >= result.a.enqueued + result.b.enqueued) { + loop.quit(); + } + }; + + QElapsedTimer clock; + QObject::connect(workerA, &CardPictureLoaderWorker::imageLoaded, &loop, + [&](const ExactCard &card, const QImage &image) { finishOne(result.a, card, image); }); + QObject::connect(workerB, &CardPictureLoaderWorker::imageLoaded, &loop, + [&](const ExactCard &card, const QImage &image) { finishOne(result.b, card, image); }); + + // Count 429 responses as seen by the shared rate limiter. + LogCounters counters; + s_activeCounters = &counters; + s_previousMessageHandler = qInstallMessageHandler(stressLogHandler); + + clock.start(); + for (const ExactCard &card : cards) { + workerA->enqueueImageLoad(card); + workerB->enqueueImageLoad(card); + } + watchdog.start(); + loop.exec(); + const qint64 elapsedMs = clock.elapsed(); + result.a.elapsedMs = elapsedMs; + result.b.elapsedMs = elapsedMs; + + // Stop both workers before touching the counters or restoring the message handler: their + // threads log from stressLogHandler, and must not outlive the stack-local counters (which is + // guaranteed on the watchdog path, where requests and deferred retries are still pending). + destroyWorker(workerA); + destroyWorker(workerB); + + result.http429Count = counters.http429; + s_activeCounters.store(nullptr, std::memory_order_release); + qInstallMessageHandler(s_previousMessageHandler); + return result; +} + +static QString formatDuration(qint64 ms) +{ + return QStringLiteral("%1.%2 s").arg(ms / 1000).arg((ms % 1000) / 100); +} + +static bool likelyRedirects(const QString &urlTemplate) +{ + return urlTemplate.contains(QStringLiteral("api.scryfall.com")); +} + +static QString hostOf(const QString &urlTemplate) +{ + return QUrl(urlTemplate).host(); +} + +static void printUsage() +{ + std::printf("usage: picture_loader_benchmark_test --carddb [options]\n" + "\n" + "Loads card pictures from the real configured hosts (not a mock server) and\n" + "verifies the picture loader's pacing / cache 429 behavior.\n" + "\n" + "options:\n" + " --carddb cards.xml to load card data from (required)\n" + " --count cards to load per template (default 300)\n" + " --url