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.

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

Merged
merged 15 commits into from
Oct 20, 2023
Merged

Conversation

arsaboo
Copy link
Contributor

@arsaboo arsaboo commented Oct 9, 2023

Description

Fixes #4942

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@sampsyo
Copy link
Member

sampsyo commented Oct 9, 2023

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 for loop? Not sure.) Would it be OK with you to start with only the first thing (the extra exception handlers) and merge that before adding retries?

Namely, these exceptions could also possibly just raise SpotifyAPIError exceptions, which already cause the plugin to harmlessly log an error and move on with its life:

beets/beetsplug/spotify.py

Lines 406 to 408 in e10b955

except SpotifyAPIError as e:
self._log.debug('Spotify API error: {}', e)
return []

Even when retries expire, this might be a friendlier thing to do than to crash with a UserError.

@arsaboo
Copy link
Contributor Author

arsaboo commented Oct 10, 2023

@sampsyo I have removed the retry logic in this PR. However, we need to fix that soon. I am increasingly seeing the Retry-After interval not being returned in the exception.

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.

Copy link
Member

@sampsyo sampsyo left a 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.

@sampsyo sampsyo merged commit efc2cf7 into beetbox:master Oct 20, 2023
14 checks passed
@arsaboo arsaboo deleted the spotify_timeout branch October 21, 2023 13:29
@arsaboo arsaboo mentioned this pull request Oct 21, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spotify does not exit gracefully when some API errors are encountered
2 participants