-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Gracefully handle Spotify API errors #4943
Conversation
Hey, thanks for getting a fix for this started!! If possible, what do you think about breaking this into a couple of steps? There's a first, more basic thing we can do here to just handle the exceptions—and then a separate, more complex matter of adding a new retry loop. The latter thing might require some care—and, in fact, there is another way to do it without implementing our own loop. (If we do the looping on our end, it might be nice to reuse the current recursive strategy rather than a Namely, these exceptions could also possibly just raise Lines 406 to 408 in e10b955
Even when retries expire, this might be a friendlier thing to do than to crash with a |
@sampsyo I have removed the retry logic in this PR. However, we need to fix that soon. I am increasingly seeing the Also, are you completely opposed to the while loop for the retry logic? Using the approach you linked will require us to change the plugin to use request sessions, which we are not doing currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you for rearranging this; this all looks right to me.
No, I'm definitely not opposed to replacing recursion with iteration for the retry loop. I just hope to be able to think carefully about both concerns separately.
Description
Fixes #4942
To Do
docs/
to describe it.)docs/changelog.rst
near the top of the document.)