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

[fetchart] Add retry with exponential backoff #4639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richardmitic
Copy link

Description

The iTunes source returns 403 as a basic form of rate limiting. To get around this, we add a retry with exponential backoff to all requests made by _logged_get(). We also include some other generic error codes to retry.

Fixes #3827, which was closed with no fix. Without this change, two different runs of beet fetchart -f will download different sets of album art because the iTunes has art available but will fail some requests with 403.

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.)

@stale
Copy link

stale bot commented May 20, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 20, 2023
Copy link
Contributor

@ybnd ybnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue as advertised, nice work!
However, a recent breaking change in urllib3 makes it so we have to jump through some hoops to get it to work.

Tested by running beet fetchart over a large library with itunes as the only source

  • Before this PR, the logs showed multiple requests failing with HTTP 403 like so
       fetchart: trying source itunes for album Chromatics - In the City
       fetchart: getting URL: https://itunes.apple.com/search?term=Chromatics+In+the+City&entity=album&media=music&limit=200
       fetchart: iTunes search failed: 403 Client Error: Forbidden for url: https://itunes.apple.com/search?term=Chromatics+In+the+City&entity=album&media=music&limit=200
       fetchart: Chromatics - In the City: no art found
    
  • With this PR, those failing requests do not appear anymore -- instead the process pauses once in a while before carrying on.

@stale
Copy link

stale bot commented Oct 15, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 15, 2023
The iTunes source returns 403 as a basic form of rate limiting. To get
around this, we add a retry with exponential backoff to all requests.
We also include some other error codes to retry.
@stale stale bot removed the stale label Oct 17, 2023
@richardmitic
Copy link
Author

@ybnd Rebased on master and removed the contentious method_whitelist/allowed_methods keyword. What do you think?

@ybnd
Copy link
Contributor

ybnd commented Oct 18, 2023

@richardmitic looks good!
I'll merge as soon as I get the chance to run this "live" again to confirm.

Copy link

stale bot commented Mar 17, 2024

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants