mirror of
https://github.com/spotipy-dev/spotipy.git
synced 2026-04-26 16:15:51 +03:00
[GH-ISSUE #577] POST playlist_add_items not retrying (HTTPError: 429 API rate limit exceeded) #344
Labels
No labels
api-bug
bug
dependencies
documentation
duplicate
enhancement
external-ide
headless-mode
implicit-grant-flow
invalid
missing-endpoint
pr-welcome
private-api
pull-request
question
spotipy3
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/spotipy#344
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @stephanebruckert on GitHub (Sep 15, 2020).
Original GitHub issue: https://github.com/spotipy-dev/spotipy/issues/577
All methods should be retrying but
playlist_add_itemsdoesn't seem to:Tried with 2.15.0 and 2.10.0
@Quizz1Cal commented on GitHub (Oct 10, 2020):
Hey @stephanebruckert I wasn't able to replicate this issue in my environment (spotipy==2.15.0, ubuntu 20.04.1 LTS, xterm) - though that might be because our networks are different. I think the root issue is that spotipy can't currently handle API rate limit exceeding as we're raising exceptions on any HTTPError. A solution might exist in the parsing of
response.json()['Retry-After'](see this link) but this would be hard to test without the error actually being thrown. Any ideas on replicable code to (quickly) exceed the rate limit?@stephanebruckert commented on GitHub (Oct 10, 2020):
Try this:
(by the way, to test it against the main branch on your local clone, the script just needs to be placed at the root of the project)
I think here
Requestsis handlingretry-afterfor us, so we shouldn't need to do anything around that:github.com/plamere/spotipy@4bb42598e1/spotipy/client.py (L201)Also it works well for GET as it correctly retries:
I wonder, why does it work for GET and not for POST, knowing that both our GET and POST methods call the same
_internal_call:github.com/plamere/spotipy@4bb42598e1/spotipy/client.py (L290)github.com/plamere/spotipy@4bb42598e1/spotipy/client.py (L295)PS: I just found this https://stackoverflow.com/a/35707701/1515819, that might be our solution!
@stephanebruckert commented on GitHub (Oct 24, 2020):
@Quizz1Cal @ritiek please review this fix if you get a chance https://github.com/plamere/spotipy/pull/596
@Beahmer89 commented on GitHub (Oct 26, 2020):
@stephanebruckert I think its worth noting that there is a reason that urllib does not do retries for POSTS and that is because they are not considered safe or idempotent. GET, PUT, and DELETE fall into these categories but POST however does not. More detail can be found in RFC7231. Having retries on POSTs can have unintended consequences like having multiple records created in the database if there was an error upon creation.
It is my personal opinion that this should not have been created as it is not an issue for the following reasons:
Please let me know if I am missing something and I am open to hearing out the reasons for the change, but I figured I would add my 2 cents as I helped contribute the change to the retry logic.
@stephanebruckert commented on GitHub (Oct 28, 2020):
Sorry for the late response I was bit busy recently. Thanks also for the inputs that's all fair points. It's my bad, I should have waited for feedback on the PR, but I'm happy to discuss it here now and am also happy to revert things if needed
Answering your points in order:
github.com/plamere/spotipy@b68a702214/spotipy/client.py (L36)I think you are right, it's probably a problem for 500, 502, 503, 504.
However my main motivation is to retry when
Retry-Afteris provided, that means when we get 429, I believe we should honourRetry-Afterand retry. Perhaps we should instead updatedefault_retry_codesand limit it to429only? Although it could be challenging to configure urllib3 to retry on all GET status codes, but only retry on POST 429s... This seems related https://github.com/urllib3/urllib3/issues/260Let's try to solve this problem in the library, so that devs don't have to worry about that exact same thing.
fair point, PUT is already part of the default, but PATCH / POST / DELETE aren't. Anyway, I wanted to "allow all", so probably we could update it to be
method_whitelist=false? if we agree on the point above (only retrying idempotent 429s)yeah I initially removed 599 because I didn't know where it came from but you are right, it's not always a 429 but one of 429, 500, 502, 503, 504. What do you think about showing the last status code we got from Spotify?
Great to discuss this!!
@stephanebruckert commented on GitHub (Oct 28, 2020):
Related to the first point about always retrying on 429, I just found we could use
respect_retry_after_headerI'm going to open a PR to set
respect_retry_after_headerto true and revert the change onmethod_whitelistas you suggestedEdit: did a quick test and doesn't seem to work
@Beahmer89 commented on GitHub (Oct 29, 2020):
@stephanebruckert no worries! Everyone gets busy and I am late to responding as well. I am glad we are talking about this as I always find this topic interesting! It seems like your main reason was to have to respect the
Retry-Afterheader if present and I think that is fair and we should do the work to ensure this is enabled for devs by default.After looking into it:
respect_retry_after_headeroption, but I believe it is enabled by default. And on further inspection it looks like it is used in these two functions is_retry and sleepis_retrychecks to make sure the retry is needed andsleepdoes the appropriate waiting. It will wait for the time that is in header or it will respect the backoff that is specified in the classAlso just to clear some things up in the comment with bullet points:
allowed_methodsparam if not specified. This param controls what methods are retried.So where does this leave us?
allowed_methodsto include the POST as well as actual idempotent defaults that urllib3 provides if it would honor the retries. I will try this today at some point hopefully.allowed_methodsto the[SpotipyClient](https://github.com/urllib3/urllib3/blob/16b7b332fd1b84c2d465f11d17658c1e83d3f20f/src/urllib3/connectionpool.py#L834-L844)that way users can programmatically add the POST method if they want to.Again really enjoying talking about this :) good stuff to think about and a fun topic!
Let me know your thoughts and if anything I said doesnt make sense or is off base!
@Mokson commented on GitHub (Nov 9, 2020):
I'm having the same issue and decided to downgrade to spotipy==2.4.4 where I know it works for sure.
retrying ...1secs
@shillshocked commented on GitHub (Nov 13, 2021):
I'm getting the same issue occasionally in my scripts. I'm wondering if it's an issue at Spotify's end?