Fix: Rate limit exceeds for Musicbrainztask. #10822

fatihemreyildiz · 2022-08-25T15:46:28Z

This PR aims to fix bug #10795 Rate Limit exceeds for some songs while Importing Metadata From Musicbrainz.

uklotzde · 2022-08-25T15:56:29Z

src/network/webtask.h

@@ -150,6 +150,8 @@ class WebTask : public NetworkTask {
        Idle,
        // Pending state
        Pending,
+        // Looping state


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.

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.

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.

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.

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.

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

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.

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.

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.

I wish you luck.

uklotzde · 2022-08-25T22:25:34Z

src/musicbrainz/web/musicbrainzrecordingstask.cpp

    }

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


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.

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.

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

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

src/network/webtask.h

src/musicbrainz/web/musicbrainzrecordingstask.h

src/musicbrainz/web/musicbrainzrecordingstask.cpp

uklotzde · 2022-08-26T11:03:37Z

@daschuer There is a cleaner, more robust and more flexible solution for what this PR tries to achieve. One of my proposals, a thoughtful and careful extension of the API and state machine that doesn't affect any other, existing classes. q.e.d.

But since decisions are now made behind the scenes... let's forget about that. Your project, your choice. Copy that.

daschuer · 2022-08-26T12:11:48Z

@uklotzde Without outlining the solution you have in mind, I am not able to compare that to the current solution.

One of my proposals, a thoughtful and careful extension of the API and state machine that doesn't affect any other, existing classes. q.e.d.

What is you proposal? Our proposal is here, proved working and with a scope suitable for a bug-fix release. You have even pointed out issues explicit.

But since decisions are now made behind the scenes... let's forget about that.

Why do you think that. All review comments are public. There is now hidden discussion in this case.

uklotzde · 2022-08-26T13:04:57Z

Why do you think that. All review comments are public. There is now hidden discussion in this case.

@daschuer You have clearly communicated that you already made decisions (not hidden, but not in the main repo and out of sight), that you are not interested in any alternative proposals, or you rejected them with unjustified arguments.

We have discussed that but rejected the idea because it either introduces code duplication or is a bugger refactoring.

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

I could easily debunk those statements with working code.

daschuer · 2022-08-26T13:16:12Z

@fatihemreyildiz I have verified whether the states are reasonable and came up with this drawing:

It turns out that there are indeed some issue with the state changes. Can you fix them?

We must not go into the Idle state in slotStart() here, because we start and go not to idle. If it fails an error state is appropriated.
https://github.com/mixxxdj/mixxx/blob/main/src/network/webtask.cpp#L185
The assertion below for idle needs to be removed

We must not go to state Pending if we have no network manager here:

mixxx/src/network/webtask.cpp

Line 189 in 669d451

m_state = State::Pending;

.
I think we can immediately move to the failed state here. The assertion in onNetworkError() needs to be adjusted accordingly.

The idle state need to be renamed to Inital. Because there is no way to get into the idle state again. Once the Task has been started.

uklotzde · 2022-08-27T09:47:19Z

The idle state need to be renamed to Inital. Because there is no way to get into the idle state again. Once the Task has been started.

@daschuer I can't face this any longer. This is how a proper solution would look like:
musicbrainz_rate_limit_fix.zip

Update: Renaming the Idle state to Initial would be ok, if it helps.

daschuer · 2022-08-27T10:25:39Z

@daschuer I can't face this any longer. This is how a proper solution would look like: musicbrainz_rate_limit_fix.zip

Can you please create a PR for this? This way we can use the normal review process. I am open to merge your PR as long as the big is fixed finally and we get out of the deadlock situation here.

uklotzde · 2022-08-27T11:06:43Z

Can you please create a PR for this? This way we can use the normal review process. I am open to merge your PR as long as the big is fixed finally and we get out of the deadlock situation here.

This patch is a gift by me to the Mixxx project. I have deleted my forked repo for a reason and don't plan to revive it under the current circumstances.

Swiftb0y · 2022-08-29T16:04:32Z

I've compared both approaches (this PR and the patch offered by @uklotzde) and I conclude that the latter one is simpler and cleaner. As pointed out previously this PR pollutes WebTask with implicit retry logic which is only used to by MusicBrainz really. Instead of making this a hidden feature, the better solution is defer a task during its creation.

Here's the patch in PR form: #10836

daschuer · 2022-09-12T10:52:27Z

I think this one is superseded by #10875
I am really sorry how the discussion has developed. Your ideas and test findings where the base of the pending PR. Thank you very much!

fatihemreyildiz · 2022-09-12T11:04:51Z

You are welcome. I am thankful for all the guidance along with the help you've did for me to trying to solve this bug and also the 404 issue. I've learned a lot meanwhile such as the Activity Diagram 'Swimlane' and UML State charts. No need to be sorry to be honest.

I'm happy that the bug is fixed. And that is all it matters IMHO 🐙

aid-mb-json-webtask: musicbrainz rate limit fixed.

518c7c2

uklotzde reviewed Aug 25, 2022

View reviewed changes

daschuer reviewed Aug 26, 2022

View reviewed changes

src/network/webtask.h Outdated Show resolved Hide resolved

daschuer reviewed Aug 26, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.h Outdated Show resolved Hide resolved

daschuer reviewed Aug 26, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.cpp Outdated Show resolved Hide resolved

acoustid-web-musicbrainz-task: state name changed.

17ddcf7

fatihemreyildiz changed the title ~~aid-mb-json-webtask: musicbrainz rate limit fixed.~~ Fix: Rate limit exceeds for Musicbrainztask. Aug 26, 2022

Idle changed to initial, no networkman no pending

ec26d8d

fatihemreyildiz added 2 commits August 27, 2022 13:15

undefined continueWithNextRequest() deleted.

29e6e55

onNetworkError state changed to Failed.

372c85f

daschuer mentioned this pull request Aug 30, 2022

Tagfetcherratelimit Swiftb0y/mixxx#9

Merged

Swiftb0y mentioned this pull request Sep 6, 2022

Uklotzde musicbrainz ratelimit patch #10836

Closed

daschuer closed this Sep 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Rate limit exceeds for Musicbrainztask. #10822

Fix: Rate limit exceeds for Musicbrainztask. #10822

fatihemreyildiz commented Aug 25, 2022

uklotzde Aug 25, 2022

daschuer Aug 25, 2022

uklotzde Aug 25, 2022

daschuer Aug 25, 2022

uklotzde Aug 25, 2022 •

edited

Loading

uklotzde Aug 26, 2022

uklotzde Aug 26, 2022 •

edited

Loading

uklotzde Aug 26, 2022

daschuer Aug 26, 2022

uklotzde Aug 26, 2022

uklotzde Aug 25, 2022

daschuer Aug 25, 2022

uklotzde Aug 26, 2022

daschuer Aug 26, 2022

uklotzde commented Aug 26, 2022

daschuer commented Aug 26, 2022

uklotzde commented Aug 26, 2022

daschuer commented Aug 26, 2022

uklotzde commented Aug 27, 2022 •

edited

Loading

daschuer commented Aug 27, 2022

uklotzde commented Aug 27, 2022 •

edited

Loading

Swiftb0y commented Aug 29, 2022

daschuer commented Sep 12, 2022

fatihemreyildiz commented Sep 12, 2022

Fix: Rate limit exceeds for Musicbrainztask. #10822

Fix: Rate limit exceeds for Musicbrainztask. #10822

Conversation

fatihemreyildiz commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Aug 26, 2022

daschuer commented Aug 26, 2022

uklotzde commented Aug 26, 2022

daschuer commented Aug 26, 2022

uklotzde commented Aug 27, 2022 • edited Loading

daschuer commented Aug 27, 2022

uklotzde commented Aug 27, 2022 • edited Loading

Swiftb0y commented Aug 29, 2022

daschuer commented Sep 12, 2022

fatihemreyildiz commented Sep 12, 2022

uklotzde Aug 25, 2022 •

edited

Loading

uklotzde Aug 26, 2022 •

edited

Loading

uklotzde commented Aug 27, 2022 •

edited

Loading

uklotzde commented Aug 27, 2022 •

edited

Loading