Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Rate limit exceeds for Musicbrainztask. #10822

Closed
wants to merge 5 commits into from
Closed

Fix: Rate limit exceeds for Musicbrainztask. #10822

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
518c7c2
aid-mb-json-webtask: musicbrainz rate limit fixed.
fatihemreyildiz Aug 25, 2022
17ddcf7
acoustid-web-musicbrainz-task: state name changed.
fatihemreyildiz Aug 26, 2022
ec26d8d
Idle changed to initial, no networkman no pending
fatihemreyildiz Aug 27, 2022
29e6e55
undefined continueWithNextRequest() deleted.
fatihemreyildiz Aug 27, 2022
372c85f
onNetworkError state changed to Failed.
fatihemreyildiz Aug 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/musicbrainz/web/acoustidlookuptask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class AcoustIdLookupTask : public network::JsonWebTask {
void emitSucceeded(
const QList<QUuid>& recordingIds);

void doLoopingTaskAborted() override{};

const QUrlQuery m_urlQuery;
};

Expand Down
56 changes: 52 additions & 4 deletions src/musicbrainz/web/musicbrainzrecordingstask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "network/httpstatuscode.h"
#include "util/assert.h"
#include "util/compatibility.h"
#include "util/duration.h"
#include "util/logger.h"
#include "util/thread_affinity.h"
#include "util/versionstore.h"
Expand All @@ -26,6 +27,8 @@ const QString kRequestPath = QStringLiteral("/ws/2/recording/");

const QByteArray kUserAgentRawHeaderKey = "User-Agent";

static constexpr int kMinTimeBetweenMbRequests = 1000;

QString userAgentRawHeaderValue() {
return VersionStore::applicationName() +
QStringLiteral("/") +
Expand Down Expand Up @@ -73,8 +76,19 @@ MusicBrainzRecordingsTask::MusicBrainzRecordingsTask(
networkAccessManager,
parent),
m_queuedRecordingIds(recordingIds),
m_measurementTimer(0),
m_parentTimeoutMillis(0) {
musicbrainz::registerMetaTypesOnce();

// According to the MusicBrainz API Doc: https://musicbrainz.org/doc/MusicBrainz_API/Rate_Limiting
// The rate limit should be one query in a second.
// Related Bug: https://github.com/mixxxdj/mixxx/issues/10795
// In order to not hit the rate limits and respect their rate limiting rule.
// Every request was delayed by one second.
connect(&m_requestTimer,
&QTimer::timeout,
this,
&MusicBrainzRecordingsTask::triggerSlotStart);
}

QNetworkReply* MusicBrainzRecordingsTask::doStartNetworkRequest(
Expand All @@ -101,10 +115,13 @@ QNetworkReply* MusicBrainzRecordingsTask::doStartNetworkRequest(
<< "GET"
<< networkRequest.url();
}

m_measurementTimer.start();

return networkAccessManager->get(networkRequest);
}

void MusicBrainzRecordingsTask::doNetworkReplyFinished(
bool MusicBrainzRecordingsTask::doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
network::HttpStatusCode statusCode) {
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
Expand All @@ -131,7 +148,7 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
statusCode),
error.code,
error.message);
return;
return true;
}

auto recordingsResult = musicbrainz::parseRecordings(reader);
Expand All @@ -153,7 +170,7 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
statusCode),
-1,
QStringLiteral("Failed to parse XML response"));
return;
return m_queuedRecordingIds.isEmpty() ? true : false;
}

if (m_queuedRecordingIds.isEmpty()) {
Expand All @@ -162,14 +179,44 @@ void MusicBrainzRecordingsTask::doNetworkReplyFinished(
auto trackReleases = m_trackReleases.values();
m_trackReleases.clear();
emitSucceeded(trackReleases);
return;
return true;
}

// Continue with next recording id
DEBUG_ASSERT(!m_queuedRecordingIds.isEmpty());
auto inhibitTimerElapsed = m_measurementTimer.elapsed(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could create multiple tasks performing concurrent requests. Adding an isolated and independent inhibit timer for each instance doesn't make any sense at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must not perform concurrent requests. Only one per second is allowed. Using multiple tasks for this is out of scope of this bugfix PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...then implement the throttling locally in the convoluted MusicBrainzRecordingsTask class and keep the base and all other classes clean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sound like a giant scope creep. No interest for this simple bugfix.

fatihemreyildiz marked this conversation as resolved.
Show resolved Hide resolved
int inhibitTimerElapsedMillis = inhibitTimerElapsed.toIntegerMillis();
qDebug() << "Task took:" << inhibitTimerElapsedMillis;
m_requestTimer.setSingleShot(true);
if (inhibitTimerElapsedMillis >= kMinTimeBetweenMbRequests) {
qDebug() << "Task took more than a second, slot is calling now.";
m_requestTimer.start(1);
} else {
auto sleepDuration = (kMinTimeBetweenMbRequests -
inhibitTimerElapsedMillis);
qDebug() << "Task took less than a second, slot is going to be called in:" << sleepDuration;
m_requestTimer.start(sleepDuration);
}
m_measurementTimer.restart(true);
return false;
}

void MusicBrainzRecordingsTask::triggerSlotStart() {
slotStart(m_parentTimeoutMillis);
}

void MusicBrainzRecordingsTask::doLoopingTaskAborted() {
kLogger.info()
<< "Aborted task was looping."
<< "Is QTimer active? (true) || (false):"
<< m_requestTimer.isActive();
if (m_requestTimer.isActive()) {
m_requestTimer.stop();
kLogger.info()
<< "QTimer is stopped.";
}
}

void MusicBrainzRecordingsTask::emitSucceeded(
const QList<musicbrainz::TrackRelease>& trackReleases) {
VERIFY_OR_DEBUG_ASSERT(
Expand All @@ -179,6 +226,7 @@ void MusicBrainzRecordingsTask::emitSucceeded(
deleteLater();
return;
}
m_requestTimer.stop();
emit succeeded(trackReleases);
}

Expand Down
10 changes: 9 additions & 1 deletion src/musicbrainz/web/musicbrainzrecordingstask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "musicbrainz/musicbrainz.h"
#include "network/webtask.h"
#include "util/timer.h"

namespace mixxx {

Expand All @@ -33,10 +34,12 @@ class MusicBrainzRecordingsTask : public network::WebTask {
QNetworkReply* doStartNetworkRequest(
QNetworkAccessManager* networkAccessManager,
int parentTimeoutMillis) override;
void doNetworkReplyFinished(
bool doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
network::HttpStatusCode statusCode) override;

void doLoopingTaskAborted() override;
fatihemreyildiz marked this conversation as resolved.
Show resolved Hide resolved

void emitSucceeded(
const QList<musicbrainz::TrackRelease>& trackReleases);
void emitFailed(
Expand All @@ -45,6 +48,7 @@ class MusicBrainzRecordingsTask : public network::WebTask {
const QString& errorMessage);

void continueWithNextRequest();
void triggerSlotStart();

const QUrlQuery m_urlQuery;

Expand All @@ -53,6 +57,10 @@ class MusicBrainzRecordingsTask : public network::WebTask {

QMap<QUuid, musicbrainz::TrackRelease> m_trackReleases;

QTimer m_requestTimer;

Timer m_measurementTimer;

int m_parentTimeoutMillis;
};

Expand Down
5 changes: 3 additions & 2 deletions src/network/jsonwebtask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ QNetworkReply* JsonWebTask::doStartNetworkRequest(
m_request.content);
}

void JsonWebTask::doNetworkReplyFinished(
bool JsonWebTask::doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
HttpStatusCode statusCode) {
DEBUG_ASSERT(finishedNetworkReply);
Expand All @@ -276,12 +276,13 @@ void JsonWebTask::doNetworkReplyFinished(
std::move(webResponse),
std::move(contentTypeAndBytes.first),
std::move(contentTypeAndBytes.second)});
return;
return true;
}
}
onFinished(JsonWebResponse{
std::move(webResponse),
optJsonContent.value_or(QJsonDocument{})});
return true;
}

void JsonWebTask::emitFailed(
Expand Down
2 changes: 1 addition & 1 deletion src/network/jsonwebtask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class JsonWebTask : public WebTask {
QNetworkReply* doStartNetworkRequest(
QNetworkAccessManager* networkAccessManager,
int parentTimeoutMillis) override;
void doNetworkReplyFinished(
bool doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
HttpStatusCode statusCode) override;

Expand Down
13 changes: 10 additions & 3 deletions src/network/webtask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ void WebTask::slotStart(int timeoutMillis) {

void WebTask::slotAbort() {
DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this);
if (m_state != State::Pending) {
if (m_state != State::Pending && m_state != State::Looping) {
DEBUG_ASSERT(m_timeoutTimerId == kInvalidTimerId);
if (m_state == State::Idle) {
kLogger.debug()
Expand All @@ -262,6 +262,10 @@ void WebTask::slotAbort() {
m_timeoutTimerId = kInvalidTimerId;
}

if (m_state == State::Looping) {
doLoopingTaskAborted();
}

auto* const pPendingNetworkReply = m_pendingNetworkReplyWeakPtr.data();
QUrl requestUrl;
if (pPendingNetworkReply) {
Expand Down Expand Up @@ -391,8 +395,11 @@ void WebTask::slotNetworkReplyFinished() {
return;
}

m_state = State::Finished;
doNetworkReplyFinished(pFinishedNetworkReply, statusCode);
if (doNetworkReplyFinished(pFinishedNetworkReply, statusCode)) {
m_state = State::Finished;
} else {
m_state = State::Looping;
}
}

} // namespace network
Expand Down
6 changes: 5 additions & 1 deletion src/network/webtask.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@ class WebTask : public NetworkTask {
Idle,
// Pending state
Pending,
// Looping state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new state does not belong to the low-level network task. WebTask has no notion of retries and doesn't need this state for implementing its internal state machine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does NOT control retries and the state IS used in WebTask itself.

It controls the new state where the WebTask is in the idle loop between two requests. Before there was no need for this state because the new request was issued just after the old one. So the solution looks like a valid one.

I am not saying this is the only possible solution, so if you like to see another solution for this Bugfix. Please give some hints how.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A task models the lifecycle of a single request. It could be restarted while not pending. Retries have to be controlled by the calling code.

Introducing a Trojan Horse into the base class just because some derived class wants to do implicit retries is the wrong approach.

I strongly recommend not to merge this PR. It introduces unneeded complexity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A task models the lifecycle of a single request.

Sorry, that is not correct. You may have designed it like that original, but since MusicBrainzRecordingsTask that inherits from WebTask works with a couple of request this no longer the case. See: MusicBrainzRecordingsTask::continueWithNextRequest();

Retries have to be controlled by the calling code.

Again, this change has nothing to do with reties.

This PR is a valid solution for a real issue, maybe not perfect. So it would be helpful to suggest improvements instead of to title it with striking words.

Copy link
Contributor

@uklotzde uklotzde Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I overlooked that. Long ago. I was confused by the wording "Looping" which is used out of context and has no meaning in the context of WebTask. This misunderstanding also reveals why adding this logic to WebTask would be wrong.

continueWithNextRequest() is undefined, probably a left over, please delete it.

I suggest to use a local timer in MusicBrainzRecordingsTask and invoke the next slotStart() with a delay. You could even override slotStart()/slotAbort() locally to detect a pending timer if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still disagree. It's a move in the wrong direction.

Copy link
Contributor

@uklotzde uklotzde Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which code duplication? It would just be a shallow wrapper around the base class slots that handles the pending timer. It is actually an supervision or overlay state and reflects the target design for a future refactoring, i.e. when extracting the looping into an orchestration task.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want to implement a throttling between subsequent requests then you could implement it entirely in the base class by adding a configuration parameter. But I don't recommend it, because the base class with its state machine is already complex.

The distributed approach introduced by this PR is hard to maintain and not needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which code duplication?

slotStart() and slotAbort()

It is actually an supervision or overlay state and reflects the target design for a future refactoring

The additional state is a much more clean and maintainable solution. Less code duplication and a clear distinguished state from the other states that actually exists. I am pretty confident with the current solution and do not plan a future refactoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish you luck.

Looping,
fatihemreyildiz marked this conversation as resolved.
Show resolved Hide resolved
// Terminal states
Aborted,
TimedOut,
Expand Down Expand Up @@ -189,8 +191,10 @@ class WebTask : public NetworkTask {
Q_UNUSED(abortedNetworkReply);
}

virtual void doLoopingTaskAborted() = 0;

/// Handle network response.
virtual void doNetworkReplyFinished(
virtual bool doNetworkReplyFinished(
QNetworkReply* finishedNetworkReply,
HttpStatusCode statusCode) = 0;

Expand Down