Adds a retry method to HttpClient

[hlapp]

Description

Allows retrying HTTP requests after they fail (status code >= 400). The parameters for retry() are modeled after httr::RETRY(), and the wait time is calculated according to the same algorithm (exponential backoff with full jitter). It also uses the same defaults.

Related Issue

See ropensci/taxize#722 for context and concrete use-case.

Example

Reproduced from the documentation:

x <- HttpClient$new(url = "https://httpbin.org")

# retry, by default at most 3 times
res_get <- x$retry("GET", path = "status/400")

# retry, but not for 404 NOT FOUND
res_get <- x$retry("GET", path = "status/404", terminate_on = c(404))

# retry, but only for exceeding rate limit (note that e.g. [Github uses 403](https://developer.github.com/v3/#rate-limiting), not [429](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429))
res_get <- x$retry("GET", path = "status/429", retry_only_on = c(403, 429))

Known limitations

• The value of the Retry-After response header, if present, is assumed to be in seconds. However, the spec allows it to be an HTTP date as well.

* There is no call back function for allowing a client to message to the user if and how much waiting is being applied.

Addresses and closes #89.

[hlapp]

FYI, the failure is in R: devel (the other two succeeded), and is from an example that wasn't modified here:

> cleanEx()
Error: connections left open:
	/tmp/Rtmp0mqmqV/file50a22ce20ab4 (file)
	/tmp/Rtmp0mqmqV/file50a2390288 (file)
	/tmp/Rtmp0mqmqV/file50a233074328 (file)
	/tmp/Rtmp0mqmqV/file50a252cdd4bc (file)
	/tmp/Rtmp0mqmqV/file50a2315f9395 (file)
	/tmp/Rtmp0mqmqV/file50a213b42482 (file)
	/tmp/Rtmp0mqmqV/file50a26b98c3ad (file)
Execution halted

Looks like open connections aren't properly cleaned up?

[hlapp]

FWIW, I'm getting a similar warning at the end of devtools::test(). I've been able to track that down to one of the 'AsyncVaried - failure behavior' tests. With this test I get the following type of warning, and without it I do not:

1: closing unused connection 4 (/var/folders/4w/1_41g8b97674d5f3x7dz9xmr0000gn/T//RtmpuDRKlo/file514e1f9c524e) 
2: closing unused connection 3 (/var/folders/4w/1_41g8b97674d5f3x7dz9xmr0000gn/T//RtmpuDRKlo/file514e23b867a4) 

Apparently, if the request doesn't complete because of things like timeout or host resolution error, an open connection may be left dangling. I think I've found where this might occur in asyncvaried.R.

I'm adding a fix for that, and then we'll see how that fares. FWIW, I actually cannot reproduce the error on the example code locally - perhaps in contrast to the Travis environment my local one is just not showing the dangling connections as an error at the end of the process. That said, the fix does make the dangling connections warning disappear at the end of devtools::test().

[codecov-io]

Codecov Report

Merging #95 into master will increase coverage by 1.01%.
The diff coverage is 97.72%.

@@            Coverage Diff            @@
##           master     #95      +/-   ##
=========================================
+ Coverage   74.28%   75.3%   +1.01%     
=========================================
  Files          22      22              
  Lines         871     911      +40     
=========================================
+ Hits          647     686      +39     
- Misses        224     225       +1

Impacted Files	Coverage Δ
R/asyncvaried.R	85.71% <100%> (+0.86%)	⬆️
R/body.R	80.3% <100%> (+0.61%)	⬆️
R/client.R	79.47% <96.87%> (+3.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc93286...1498327. Read the comment docs.

[hlapp]

The failure in R: devel on checking the examples remains, pretty much unchanged. However, I suspect that has something to do with the fact that crul gets installed from CRAN as a package dependency? The sessionInfo() log shows crul at 0.6.0, the CRAN version, not the development version.

[hlapp]

I notice that the issue behind the error has been reported previously as #93.

[sckott]

Thanks @hlapp for this. Been meaning to get around to it, but just haven't found the time.

The connections left open I also meant to figure out, thanks for looking into that.

Having a look

[sckott]

closing connections isn't totally solved, but i'll try to sort it out asap.

[sckott]

for these two

• The value of the Retry-After response header, if present, is assumed to be in seconds. However, the spec allows it to be an HTTP date as well.
• There is no call back function for allowing a client to message to the user if and how much waiting is being applied.

does it seem reasonable to open issues and handle later? or do you think they're essential for any usage?

[hlapp]

does it seem reasonable to open issues and handle later? or do you think they're essential for any usage?

I don't think they're essential, and opening issues to track them for handling later would seem quite reasonable to be to have something in place that works for probably the most common use-cases.

In fact, I'm not aware yet of an API that does actually give the Retry-After as a date rather than seconds. Would always be good to first have a concrete use-case against which to check date parsing, for example.

As for enabling a client to message to the user, I did this initially in a different (non-work, JavaScript) application, and turned it off soon because it was just amounting to noise. I can, however, imagine this being somewhat useful for a client doing, for example, un-authenticated Github API requests and who then after 60 requests has to wait for almost an hour until the rate limit resets. Arguably, even then though it would be better to just get an API key and authenticate requests with it.

[hlapp]

closing connections isn't totally solved, but i'll try to sort it out asap.

I don't know how far you've gotten with this. I had another look too, and could find only two places where connections that were opened might under some circumstances end up dangling. The change I added in c159bba should close those (potential) holes.

I can't test though whether these have any positive impact on the problem, because I can't reproduce what's showing on Travis, including not on a Ubuntu 16.04 machine. (The problem for running the examples doesn't even surface for master, which has neither of the two fixes.)

[hlapp]

🎉 🌮 🎉 OMG it passed!!!

[hlapp]

• There is no call back function for allowing a client to message to the user if and how much waiting is being applied.

Just added that too while I was at it.

Note BTW that if Reset-After is a date rather than seconds, it shouldn't fail silently. (as.numeric() should fail.) So hopefully this will be reported if encountered in the wild.

[sckott]

Opened an issue for the retry-after date issue #96

[sckott]

I hadn't worked on the connections issue yet, so thanks for that. Connections seem to be good now

[sckott]

Thanks for this @hlapp - merging now