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.
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
Implement retry on 503 server response, using Retry-After
Header value
#13
Comments
At the time I opened this issue I had not yet written the user agent feature, so I didn't thought about doing it this way. That's a great idea, I will try that when I have some spare time. Or maybe you would like to submit a PR ? |
Should we simply include headers in every request by default now that we have the user agent feature? In the docs under #User-Agent, it mentions:
I glanced at the source code to python-musicbrainz2 and it indeed sets the user agent by default to So, similarly perhaps we could default to something like That said, but now in python-musicbrainz3 (which aims to be a successor to Also from the docs here, it mentions:
Is this maybe a hint that all future official libraries are now supposed to force users to set their own user agent? In this case, we should probably go this route if being an official rust client is on our roadmap. |
I guess it's worth trying to set the user agent in the integration tests and see if it does not hit rate limiting, but I am afraid it will still fails due to IP blocking/throttling rules. Being an official rust client is definitely on our roadmap but I am not sure setting the user agent should be mandatory. I think we shall discuss this further with musicbrainz people. For now we can simply try to fix the test suite, I will ask on the IRC if we should use a default |
Alright!
I just tried this out locally and also on my fork in ritiek/musicbrainz_rs#2 (to see how GitHub actions behaves) by adding a user-agent and removing the 1s sleep from all tests. You are right about tests still failing due to IP throttling.
This could be a way out maybe. However, when MusicBrainz hits the rate-limit, they seem to only return the HTTP 503 error code, but nothing else useful in the response content such as how much time to wait before making the next query that wouldn't be rate-limited. We probably may have to sleep for small random durations before making a retry query if we are to implement this I think, which I'm not sure MusicBrainz people will be happy with. |
No response from musicbrainz people so far, I guess we will stick with the default header for now and let users set the user-agent if needed. Anyway I am not comfortable with making the user agent header mandatory. As for the retry on 503 response, this is actually a good idea. I just checked, musicbrainz server send a |
Retry-After
Header value
Wow, that's cool! I shouldn't have overlooked the response headers. |
Rate limiting cause integration tests to fail randomly, for now a quick'n dirty fix has been made with thread sleeps on every test.
Adding a proper user agent header as described in MusicBrainz rate limiting documentation might solve the issue.
This still need to be figured out but implementing a retry on
HTTP 503
responses might also be a good idea.The text was updated successfully, but these errors were encountered: