Merge pull request #10874 from uklotzde/uklotzde-musicbrainz-ratelimi…

…t-patch

Fetch metadata from MusicBrainz with adaptive rate limiting

src/musicbrainz/web/musicbrainzrecordingstask.cpp

@@ -26,6 +26,11 @@ const QString kRequestPath = QStringLiteral("/ws/2/recording/");
 
 const QByteArray kUserAgentRawHeaderKey = "User-Agent";
 
+// MusicBrainz allows only a single request per second on average
+// to avoid rate limiting.
+// See: <https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting>
+constexpr Duration kMinDurationBetweenRequests = Duration::fromMillis(1000);
+
 QString userAgentRawHeaderValue() {
     return VersionStore::applicationName() +
             QStringLiteral("/") +
@@ -101,6 +106,7 @@ QNetworkReply* MusicBrainzRecordingsTask::doStartNetworkRequest(
                 << "GET"
                 << networkRequest.url();
     }
+    m_lastRequestSentAt.start();
     return networkAccessManager->get(networkRequest);
 }
 
@@ -167,7 +173,15 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
 
     // Continue with next recording id
     DEBUG_ASSERT(!m_queuedRecordingIds.isEmpty());
-    slotStart(m_parentTimeoutMillis);
+
+    // Ensure that at least kMinDurationBetweenRequests has passed
+    // since the last request before starting the next request.
+    // This is achieved by adjusting the start delay adaptively.
+    const Duration elapsedSinceLastRequestSent = m_lastRequestSentAt.elapsed();
+    const Duration delayBeforeNextRequest =
+            kMinDurationBetweenRequests -
+            std::min(kMinDurationBetweenRequests, elapsedSinceLastRequestSent);
+    slotStart(m_parentTimeoutMillis, delayBeforeNextRequest.toIntegerMillis());
 }
 
 void MusicBrainzRecordingsTask::emitSucceeded(

src/musicbrainz/web/musicbrainzrecordingstask.h

@@ -8,6 +8,7 @@
 
 #include "musicbrainz/musicbrainz.h"
 #include "network/webtask.h"
+#include "util/performancetimer.h"
 
 namespace mixxx {
 
@@ -44,15 +45,15 @@ class MusicBrainzRecordingsTask : public network::WebTask {
             int errorCode,
             const QString& errorMessage);
 
-    void continueWithNextRequest();
-
     const QUrlQuery m_urlQuery;
 
     QList<QUuid> m_queuedRecordingIds;
     QSet<QUuid> m_finishedRecordingIds;
 
     QMap<QUuid, musicbrainz::TrackRelease> m_trackReleases;
 
+    PerformanceTimer m_lastRequestSentAt;
+
     int m_parentTimeoutMillis;
 };
 

src/network/networktask.cpp

@@ -31,16 +31,17 @@ NetworkTask::~NetworkTask() {
     s_instanceCounter.increment(-1);
 }
 
-void NetworkTask::invokeStart(int timeoutMillis) {
+void NetworkTask::invokeStart(int timeoutMillis, int delayMillis) {
     QMetaObject::invokeMethod(
             this,
 #if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
             "slotStart",
             Qt::AutoConnection,
-            Q_ARG(int, timeoutMillis)
+            Q_ARG(int, timeoutMillis),
+            Q_ARG(int, delayMillis)
 #else
-            [this, timeoutMillis] {
-                this->slotStart(timeoutMillis);
+            [this, timeoutMillis, delayMillis] {
+                this->slotStart(timeoutMillis, delayMillis);
             }
 #endif
     );

src/network/networktask.h

@@ -59,24 +59,32 @@ class NetworkTask : public QObject {
 
   public:
     static constexpr int kNoTimeout = 0;
+    static constexpr int kNoStartDelay = 0;
 
     /// Start a new task by sending a network request.
     ///
     /// timeoutMillis <= 0: No timeout (unlimited)
-    /// timeoutMillis > 0: Implicitly aborted after timeout expired
+    /// timeoutMillis > 0:  Implicitly aborted after timeout expired
+    ///
+    /// delayMillis <= 0: Send request immediately
+    /// delayMillis > 0:  Send request after a delay, e.g. for rate limiting subsequent requests
     ///
     /// This function is thread-safe and could be invoked from any thread.
     void invokeStart(
-            int timeoutMillis = kNoTimeout);
+            int timeoutMillis = kNoTimeout,
+            int delayMillis = kNoStartDelay);
 
     /// Cancel the task by aborting the pending network request.
     ///
     /// This function is thread-safe and could be invoked from any thread.
     void invokeAbort();
 
   public slots:
+    /// See also: invokeStart()
     virtual void slotStart(
-            int timeoutMillis) = 0;
+            int timeoutMillis = kNoTimeout,
+            int delayMillis = kNoStartDelay) = 0;
+    /// See also: invokeAbort()
     virtual void slotAbort() = 0;
 
   signals:

src/network/webtask.cpp

@@ -113,8 +113,9 @@ WebTask::WebTask(
         QNetworkAccessManager* networkAccessManager,
         QObject* parent)
         : NetworkTask(networkAccessManager, parent),
-          m_state(State::Idle),
-          m_timeoutTimerId(kInvalidTimerId) {
+          m_state(State::Initial),
+          m_timeoutTimerId(kInvalidTimerId),
+          m_timeoutMillis(kNoTimeout) {
     std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce);
 }
 
@@ -171,9 +172,9 @@ void WebTask::emitNetworkError(
             responseWithContent);
 }
 
-void WebTask::slotStart(int timeoutMillis) {
+void WebTask::slotStart(int timeoutMillis, int delayMillis) {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (m_state == State::Pending) {
+    if (isBusy()) {
         kLogger.warning()
                 << "Task is still busy and cannot be started again";
         return;
@@ -182,7 +183,25 @@ void WebTask::slotStart(int timeoutMillis) {
     // Reset state
     DEBUG_ASSERT(!m_pendingNetworkReplyWeakPtr);
     DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
-    m_state = State::Idle;
+    m_state = State::Initial;
+    m_timeoutMillis = kNoTimeout;
+
+    if (delayMillis > 0) {
+        m_state = State::Starting;
+        kLogger.debug()
+                << this
+                << "Scheduling next request after" << delayMillis << "ms";
+        DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
+        // When the task is is starting/delayed, the timeoutTimer
+        // is used for scheduling the request that should happen
+        // after the delay. Afterwards, the timemoutTimer is used for
+        // the actual request timeout.
+        m_timeoutTimerId = startTimer(delayMillis);
+        DEBUG_ASSERT(m_timeoutTimerId != kInvalidTimerId);
+        // Store timeout for later
+        m_timeoutMillis = timeoutMillis;
+        return;
+    }
 
     auto* const pNetworkAccessManager = m_networkAccessManagerWeakPtr.data();
     VERIFY_OR_DEBUG_ASSERT(pNetworkAccessManager) {
@@ -208,7 +227,7 @@ void WebTask::slotStart(int timeoutMillis) {
     // during the callback before it has beeen started
     // successfully. Instead it should return nullptr
     // to abort the task immediately.
-    DEBUG_ASSERT(m_state == State::Idle);
+    DEBUG_ASSERT(m_state == State::Initial);
     if (!m_pendingNetworkReplyWeakPtr) {
         kLogger.debug()
                 << this
@@ -225,6 +244,7 @@ void WebTask::slotStart(int timeoutMillis) {
         DEBUG_ASSERT(timeoutMillis > 0);
         m_timeoutTimerId = startTimer(timeoutMillis);
         DEBUG_ASSERT(m_timeoutTimerId != kInvalidTimerId);
+        m_timeoutMillis = timeoutMillis;
     }
 
     // It is not necessary to connect the QNetworkReply::errorOccurred signal.
@@ -238,9 +258,9 @@ void WebTask::slotStart(int timeoutMillis) {
 
 void WebTask::slotAbort() {
     DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
-    if (m_state != State::Pending) {
+    if (!isBusy()) {
         DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
-        if (m_state == State::Idle) {
+        if (m_state == State::Initial) {
             kLogger.debug()
                     << this
                     << "Cannot abort idle task";
@@ -260,11 +280,13 @@ void WebTask::slotAbort() {
     if (m_timeoutTimerId != kInvalidTimerId) {
         killTimer(m_timeoutTimerId);
         m_timeoutTimerId = kInvalidTimerId;
+        m_timeoutMillis = kNoTimeout;
     }
 
-    auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
     QUrl requestUrl;
+    auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
     if (pPendingNetworkReply) {
+        DEBUG_ASSERT(m_state == State::Pending);
         if (pPendingNetworkReply->isRunning()) {
             kLogger.debug()
                     << this
@@ -308,6 +330,13 @@ void WebTask::timerEvent(QTimerEvent* event) {
     killTimer(m_timeoutTimerId);
     m_timeoutTimerId = kInvalidTimerId;
 
+    if (m_state == State::Starting) {
+        DEBUG_ASSERT(!m_pendingNetworkReplyWeakPtr);
+        m_state = State::Initial;
+        slotStart(m_timeoutMillis);
+        return;
+    }
+
     if (hasTerminated()) {
         DEBUG_ASSERT(!m_pendingNetworkReplyWeakPtr);
         return;

src/network/webtask.h

@@ -136,7 +136,8 @@ class WebTask : public NetworkTask {
 
   public slots:
     void slotStart(
-            int timeoutMillis) override;
+            int timeoutMillis = kNoTimeout,
+            int delayMillis = kNoStartDelay) override;
     void slotAbort() override;
 
   private slots:
@@ -147,7 +148,9 @@ class WebTask : public NetworkTask {
 
     enum class State {
         // Initial state
-        Idle,
+        Initial,
+        // Timed start
+        Starting,
         // Pending state
         Pending,
         // Terminal states
@@ -161,6 +164,11 @@ class WebTask : public NetworkTask {
         return m_state;
     }
 
+    bool isBusy() const {
+        return state() == State::Starting ||
+                state() == State::Pending;
+    }
+
     bool hasTerminated() const {
         return state() == State::Aborted ||
                 state() == State::TimedOut ||
@@ -210,6 +218,7 @@ class WebTask : public NetworkTask {
     PerformanceTimer m_timer;
 
     int m_timeoutTimerId;
+    int m_timeoutMillis;
 
     SafeQPointer<QNetworkReply> m_pendingNetworkReplyWeakPtr;
 };