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

use RETRY logic for HTTP calls #1305

Merged
merged 2 commits into from
May 23, 2020
Merged

use RETRY logic for HTTP calls #1305

merged 2 commits into from
May 23, 2020

Conversation

jameslamb
Copy link
Contributor

Thanks for for this awesome project!

In this PR, I'd like to propose swapping out calls to httr::<verb>() etc. with httr::RETRY(). This will make the package more resilient to transient problems like brief network outages or periods where the service(s) it hits are overwhelmed. In my experience, using retry logic almost always improves the user experience with HTTP clients.

I'm working on chircollab/chircollab20#1 as part of Chicago R Collab, an R 'unconference' in Chicago.

@jayhesselberth
Copy link
Collaborator

This is a nice idea, but I have never run into connectivity issues, and in general we shouldn't be fixing problems that don't exist because it can cause maintainability issues down the line. That said, it's possible this is a completely inert change.

@jameslamb
Copy link
Contributor Author

This is a nice idea, but I have never run into connectivity issues, and in general we shouldn't be fixing problems that don't exist because it can cause maintainability issues down the line. That said, it's possible this is a completely inert change.

Fair concern! In my experience, adding retry logic almost always improves the user experience with HTTP clients, especially when it's done well and test by another another project so you don't have to implement it yourself.

httr::RETRY() can be a drop-in replacement for GET(), POST(), etc. so I don't think it should change your maintenance burden. We've been using it in uptasticsearch for about 9 months without issue.

httr::RETRY() implements exponential backoff, a best practice used to ensure that retry attempts don't make the situation on the server worse. It also has handling for the special Retry-After header, so if a server sends back a response that says "hey I can't handle this right now, wait 38 seconds", it will do that.

All that said...I'll of course respect whatever you want to do with this PR. Thanks for the time and consideration!

@hadley
Copy link
Member

hadley commented May 22, 2020

I think it's a relatively harmless change, so if you add a news bullet (which should briefly describe the change and end with (@yourname, #issuenumber)) I think we can merge.

@jameslamb
Copy link
Contributor Author

I think it's a relatively harmless change, so if you add a news bullet (which should briefly describe the change and end with (@yourname, #issuenumber)) I think we can merge.

ok just added a NEWS item!

@jayhesselberth jayhesselberth merged commit ac78596 into r-lib:master May 23, 2020
@jayhesselberth
Copy link
Collaborator

Thanks!

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.

3 participants