added retry logic for all HTTP requests

[jameslamb]

Thanks for this great package!

In this PR, I'd like to propose swapping out calls to httr::POST(), httr::GET(), 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.

[mpadge]

Thanks @jameslamb for the useful prompt to ensure that RETRY is used throughout. You'll note that it was already used in opq(), but I had neglected to use RETRY in the main overpass_query() function. I'll write some comments in your code to ask you to address a few issues there, then be happy to merge.

---

EDIT: Actually, RETRY ought not really be used except where it already was. Can you please remove ALL instances from the tests, and revert them to all to GET. We'll have a look at what remains after that, and think about PR'ing once those changes have been reverted. Thanks

[jameslamb]

Thanks @jameslamb for the useful prompt to ensure that RETRY is used throughout. You'll note that it was already used in opq(), but I had neglected to use RETRY in the main overpass_query() function. I'll write some comments in your code to ask you to address a few issues there, then be happy to merge.

EDIT: Actually, RETRY ought not really be used except where it already was. Can you please remove ALL instances from the tests, and revert them to all to GET. We'll have a look at what remains after that, and think about PR'ing once those changes have been reverted. Thanks

sure, not a problem. Could you explain why GET() is more appropriate than RETRY("GET") in those tests?

[mpadge]

The RETRY verb is just a wrapper around some R-specific code within the httr package to control multiple GET attempts, just as is currently coded explicitly in the if (wait) clause of overpass_query(). This function uses RETRY is a slot is available, but on subsequent ones reverts to GET because the API itself dictates acceptable frequencies for retry.

The tests are mocked, so they do not execute real http requests. A "recording" has to be made of a successful response, and then the test uses GET to simulate what would happen in a real http call. The thing being mocked is is the underlying libcurl verb, which is GET. RETRY is just a thin wrapper within an R package for automatically implementing a scheme for multiple GET attempts, and has nothing to do with standard http protocols, which only know two verbs, GET and POST. In short: the things being mocked are the contents of a http call as returned directly from libcurl, which only knows two verbs, GET and POST. These contents have nothing to do with R, nothing to do with RETRY, and are precisely equal to nothing other than the binary result of a http GET call. And that verb is accordingly the only sensible thing to use.

[jameslamb]

The RETRY verb is just a wrapper around some R-specific code within the httr package to control multiple GET attempts, just as is currently coded explicitly in the if (wait) clause of overpass_query(). This function uses RETRY is a slot is available, but on subsequent ones reverts to GET because the API itself dictates acceptable frequencies for retry.

The tests are mocked, so they do not execute real http requests. A "recording" has to be made of a successful response, and then the test uses GET to simulate what would happen in a real http call. The thing being mocked is is the underlying libcurl verb, which is GET. RETRY is just a thin wrapper within an R package for automatically implementing a scheme for multiple GET attempts, and has nothing to do with standard http protocols, which only know two verbs, GET and POST. In short: the things being mocked are the contents of a http call as returned directly from libcurl, which only knows two verbs, GET and POST. These contents have nothing to do with R, nothing to do with RETRY, and are precisely equal to nothing other than the binary result of a http GET call. And that verb is accordingly the only sensible thing to use.

I disagree that RETRY() is a "thin" wrapper. It implements exponential backoff and is smart enough to respect the Retry-After header on 429s (code).

I disagree that HTTP has "...only two known verbs, GET and POST". The HTTP spec defines 8 methods.

Mocking httr::GET() is not correct in the case that you're using httr::RETRY("GET", ...) to issue GET requests. That function is not called internally by httr::RETRY().

httr::RETRY() (code):

req <- request_build(verb, hu$url, body_config(body, match.arg(encode)), config, ...)

httr::GET() (code)

req <- request_build("GET", hu$url, as.request(config), ...)

[mpadge]

What would you like to do then? The PR as submitted was inappropriate because the tests can not just be changed like that, so it has to be fixed. Of course there are more http verbs, but there are only two fetch verbs. And any wrapper in R that serves to change the way external library calls are handled and processed can of course be argued to be "thin" - the only layer in which RETRY makes any sense is within R; the moment those calls are passed to C (the language of libcurl), they can only be either GET or POST. And i don't get the point of your code quotes at the end - httr::GET of course just passes the http GET verb, whereas RETRY can accept either GET or POST, so passes a generic verb. For httr::RETRY(verb = "GET", ...), the value of verb is GET, and that's what is called.

I'm happy to work with you on this, but less disagreement would be helpful, as would thinking a bit more about the underlying code structure.

[jameslamb]

less disagreement would be helpful

I'll close this pull request. Have a good week.

[mpadge]

no worries - please feel free to contribute any other way you might like. Thanks for the interest and input regardless 😄