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

Don't retry on selected statuses #404

Closed
leonidb opened this Issue Oct 26, 2016 · 12 comments

Comments

Projects
None yet
4 participants
@leonidb

leonidb commented Oct 26, 2016

No point to retry on certain statuses, like 401 or 403 for example.
And there is no option to customize on which statuses to retry.

@asieira

This comment has been minimized.

Contributor

asieira commented Mar 5, 2017

I have submitted a new pull request that addresses this and also scenarios where the underlying curl code can raise errors as well: #429

@patr1ckm

This comment has been minimized.

patr1ckm commented Jul 12, 2017

It's humorous that this issue is #404

@hadley

This comment has been minimized.

Member

hadley commented Jul 24, 2017

I'm not sure what the API should look like here. Maybe a vector of additional status codes that should terminate the retry cycle?

@hadley hadley added the feature label Jul 24, 2017

@asieira

This comment has been minimized.

Contributor

asieira commented Jul 24, 2017

That is basically what I did on the PR I submitted before, with the addition of the optional use of try to catch exceptions raised by the underlying curl code.

Another option that would be more flexible but IMHO less user-friendly would be to request a function parameter that would be applied to the response object and that should return a logical scalar to indicate if the retries should continue.

@hadley

This comment has been minimized.

Member

hadley commented Jul 24, 2017

@asieira you called it "safe statuses" which is confusing (because many aren't actually safe) and the logic you used ((!status_code(resp) %in% safe_statuses && http_error(resp)) is confusing. You also tangled it up with another unrelated issue.

@leonidb

This comment has been minimized.

leonidb commented Jul 24, 2017

In practice the desired behavior would depend a lot on service implementation and client's use case. Probably for some use cases might be useful to decide on the retry based on the response body (not saying it's a recommended practice, but it might be the case).

So, despite what I stated originally when I've opened this issue, I think the more flexible option of a function parameter would probably be preferable.

Additionally, it makes sense and is often useful to retry on connection errors, to overcome a flaky connection. Yet for other possible curl exceptions it makes less sense. Not sure it's worth yet additional parameter - a list of exception types to retry on, but maybe worth to hand pick the ones that are useful for retries.

@patr1ckm

This comment has been minimized.

patr1ckm commented Jul 24, 2017

Being able to provide a vector of additional status codes that terminate the retry cycle is all I need.

@asieira

This comment has been minimized.

Contributor

asieira commented Jul 25, 2017

@leonidb I've seen my fair share of temporary DNS failures to resolve hostnames be raised as exceptions by curl as well, not only connection failures. So that's why I suggested using try to catch and retry on any of them.

@asieira

This comment has been minimized.

Contributor

asieira commented Jul 25, 2017

@hadley thank you for the feedback, happy to improve on that and re-submit. 🙇

@hadley

This comment has been minimized.

Member

hadley commented Jul 25, 2017

Let's start with a vector of additional status codes (i.e. codes in the 400-500) range to return on. I'm not sure exactly what it should be called but maybe something like terminate_on?

@asieira another attempt would be great. Can you please use tryCatch() instead of try()?

@asieira

This comment has been minimized.

Contributor

asieira commented Jul 25, 2017

Question - if the last response after all of the retries are finished is still an error condition (as caught with tryCatch), should RETRY return that or should it call stop with it to remain backwards compatible?

My vote goes to raising it again so as to avoid breaking existing code, but wanted to check what you think.

asieira added a commit to asieira/httr that referenced this issue Jul 25, 2017

Improvements to RETRY (r-lib#404)
* Add new `terminate_on` parameter that allows status_codes that
prevent retries to be specified;
* Catch error conditions using `tryCatch` and retry if they occur.
@asieira

This comment has been minimized.

Contributor

asieira commented Jul 25, 2017

@hadley hopefully this is better: #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment