-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Retry failed HTTP(S) fetches when appropriate. #118
Conversation
@achimnol if you could take a look, I'd appreciate it. |
reraise=True, | ||
retry=retry_if_exception( | ||
lambda ex: ( | ||
isinstance(ex, TimeoutException) |
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.
N.B.: I tested most of this config by using a temporary low timeout on the httpx.Client
. You do get 2 warning logs before the 2 retries and the final exception is the timeout of the last try.
@@ -99,6 +135,7 @@ def _fetch_to_cache( | |||
configured_client(url, headers).stream("GET", url) as response, | |||
work.open("wb") as cache_fp, | |||
): | |||
response.raise_for_status() |
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.
This missing response.raise_for_status
and the one added below were likely bugs that may combine with the rest of this code to actually fix the OP issue. Discussion here: #116 (comment)
Even if not, I'd like to proceed with the change since retries seem like a good idea to me independently of #116.
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.
Makes sense. The fail-open API of requiring an explicit raise_for_status
on each response is definitely annoying/makes it easy to miss!
Fixes #116