From c1332c5b983053285eeed09c91ad302134d6f7ae Mon Sep 17 00:00:00 2001 From: askmeaboutloom Date: Tue, 1 Oct 2024 14:46:50 +0200 Subject: [PATCH] Don't use weird recursion to refresh announcements Because that can lead to an infinite loop that leads to the server not feeling well. The whole reason it was implemented like this was to bucket session announcement refreshes per list server, which is a pretty goofy way to do it. Now it just builds a map of lists instead. --- ChangeLog | 1 + src/libserver/announcements.cpp | 212 ++++++++++++++++---------------- 2 files changed, 109 insertions(+), 104 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5aa482fcd..ba211d0a2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -31,6 +31,7 @@ Unreleased Version 2.2.2-pre * Fix: In layer view mode, render the layer truly in isolation instead of applying opacities, visibilities or alpha preserve to it. Thanks MachKerman and incoheart for reporting. * Feature: Add Layer > Group View to show the parent group of the current layer in isolation. Thanks Rylan for suggesting. * Fix: Save and export images according to the current view mode. Thanks incoheart for reporting. + * Server Fix: Don't get announcement refreshes stuck in an infinite loop. Thanks Meru for reporting. 2024-08-09 Version 2.2.2-beta.3 * Fix: Use more accurate timers for performance profiles if the platform supports it. diff --git a/src/libserver/announcements.cpp b/src/libserver/announcements.cpp index 0378fa17f..e03c52bcf 100644 --- a/src/libserver/announcements.cpp +++ b/src/libserver/announcements.cpp @@ -171,120 +171,124 @@ void Announcements::timerEvent(QTimerEvent *event) void Announcements::refreshListings() { - QVector> updates; - QUrl refreshServer; - bool needsSecondPass = false; + using Update = QPair; + using Updates = QVector; + using UpdatesByServer = QHash; - // Gather a list of announcements that need refreshing + UpdatesByServer serverUpdates; for(Listing &listing : m_announcements) { - bool shouldRefresh = listing.finishedListing && ( - listing.refreshTimer.hasExpired(listing.announcement.refreshInterval * 60 * 1000) || - listing.session->hasUrgentAnnouncementChange(listing.description) || - refreshServer == listing.listServer); - + bool shouldRefresh = + listing.finishedListing && + (listing.refreshTimer.hasExpired( + listing.announcement.refreshInterval * 60 * 1000) || + listing.session->hasUrgentAnnouncementChange(listing.description)); if(shouldRefresh) { - Q_ASSERT(listing.announcement.apiUrl == listing.listServer); - - // The bulk update function can only update one server at a time. - // Therefore, if there are refreshable listings for more than one server, - // we'll call refreshListings recursively until everything has been updated. - // Also, if there is at least one session that needs refreshing, we'll refresh - // all sessions listed at the same listserver. This way, the refresh intervals - // sync up and we minimize the number of requests we need to make. - if(!refreshServer.isValid()) { - refreshServer = listing.listServer; - - } else if(refreshServer != listing.listServer) { - needsSecondPass = true; - continue; - } - - Session description = listing.session->getSessionAnnouncement(); - - updates << QPair { - listing.announcement, - description - }; - + serverUpdates[listing.listServer].append( + {listing.announcement, + listing.session->getSessionAnnouncement()}); listing.refreshTimer.start(); } } - if(updates.isEmpty()) - return; - - // Bulk refresh - server::Log() - .about(server::Log::Level::Info, server::Log::Topic::PubList) - .message(QStringLiteral("Refreshing %1 announcements at %2").arg(updates.size()).arg(updates.first().first.apiUrl.toString())) - .to(m_config->logger()); - - auto *response = sessionlisting::refreshSessions(updates); - - connect(response, &AnnouncementApiResponse::finished, [this, refreshServer, updates, response](const QVariant &result, const QString &message, const QString &error) { - response->deleteLater(); - - if(!message.isEmpty()) { - server::Log() - .about(server::Log::Level::Info, server::Log::Topic::PubList) - .message(message) - .to(m_config->logger()); - } - - if(!error.isEmpty()) { - // If bulk refresh fails, there is a serious problem - // with the server. Remove all listings. - server::Log() - .about(server::Log::Level::Warn, server::Log::Topic::PubList) - .message(refreshServer.toString() + ": bulk refresh error: " + error) - .to(m_config->logger()); - - unlistSession(nullptr, refreshServer, false); - return; - } + for(UpdatesByServer::const_iterator it = serverUpdates.constBegin(), + end = serverUpdates.constEnd(); + it != end; ++it) { - const QVariantHash results = result.toHash(); - const QVariantHash responses = results["responses"].toHash(); - const QVariantHash errors = results["errors"].toHash(); + const QUrl &refreshServer = it.key(); + const Updates &updates = it.value(); - for(const auto &pair : updates) { - Q_ASSERT(pair.first.apiUrl == refreshServer); - - Listing *listing = findListingById(refreshServer, pair.first.listingId); - if(!listing) { - // Whoops! Looks like this session has ended - continue; - } - - listing->description = pair.second; - - // Remove missing listings - const QString listingId = QString::number(listing->announcement.listingId); - const QString resultValue = responses.value(listingId).toString(); - if(resultValue != "ok") { - QString errorMessage = errors[listingId].toString(); - server::Log() - .about(server::Log::Level::Warn, server::Log::Topic::PubList) - .session(listing->announcement.id) - .message(QStringLiteral("%1: %2 (%3)").arg( - listing->listServer.toString(), resultValue, - errorMessage.isEmpty() ? "no error message" : errorMessage)) - .to(m_config->logger()); - - QString announcementErrorMessage = - QStringLiteral("The session has been delisted from %1: %2").arg( - listing->listServer.host(), - errorMessage.isEmpty() ? "no reason given" : errorMessage); - emit announcementError(listing->session, announcementErrorMessage); - - unlistSession(listing->session, listing->listServer, false); - } - } - }); + server::Log() + .about(server::Log::Level::Info, server::Log::Topic::PubList) + .message(QStringLiteral("Refreshing %1 announcements at %2") + .arg(updates.size()) + .arg(updates.first().first.apiUrl.toString())) + .to(m_config->logger()); - // If there were refreshes for more than one server, do them next - if(needsSecondPass) - refreshListings(); + AnnouncementApiResponse *response = + sessionlisting::refreshSessions(updates); + + connect( + response, &AnnouncementApiResponse::finished, + [this, refreshServer, updates, response]( + const QVariant &result, const QString &message, + const QString &error) { + response->deleteLater(); + + if(!message.isEmpty()) { + server::Log() + .about( + server::Log::Level::Info, + server::Log::Topic::PubList) + .message(message) + .to(m_config->logger()); + } + + if(!error.isEmpty()) { + // If bulk refresh fails, there is a serious problem + // with the server. Remove all listings. + server::Log() + .about( + server::Log::Level::Warn, + server::Log::Topic::PubList) + .message(QStringLiteral("%1: bulk refresh error: %2") + .arg(refreshServer.toString(), error)) + .to(m_config->logger()); + unlistSession(nullptr, refreshServer, false); + return; + } + + const QVariantHash results = result.toHash(); + const QVariantHash responses = results["responses"].toHash(); + const QVariantHash errors = results["errors"].toHash(); + for(const Update &pair : updates) { + Q_ASSERT(pair.first.apiUrl == refreshServer); + + Listing *listing = + findListingById(refreshServer, pair.first.listingId); + if(!listing) { + // Whoops! Looks like this session has ended + continue; + } + + listing->description = pair.second; + + // Remove missing listings + const QString listingId = + QString::number(listing->announcement.listingId); + const QString resultValue = + responses.value(listingId).toString(); + if(resultValue != QStringLiteral("ok")) { + QString errorMessage = errors[listingId].toString(); + server::Log() + .about( + server::Log::Level::Warn, + server::Log::Topic::PubList) + .session(listing->announcement.id) + .message(QStringLiteral("%1: %2 (%3)") + .arg( + listing->listServer.toString(), + resultValue, + errorMessage.isEmpty() + ? "no error message" + : errorMessage)) + .to(m_config->logger()); + + QString announcementErrorMessage = + QStringLiteral( + "The session has been delisted from %1: %2") + .arg( + listing->listServer.host(), + errorMessage.isEmpty() ? "no reason given" + : errorMessage); + emit announcementError( + listing->session, announcementErrorMessage); + + unlistSession( + listing->session, listing->listServer, false); + } + } + }); + } } QVector Announcements::getAnnouncements(const Announcable *session) const