Skip to content

Commit

Permalink
By default, don't retry on curl failures
Browse files Browse the repository at this point in the history
Since they're not usually resolvable just by waiting, and it's better to give a clear error.
  • Loading branch information
hadley committed Sep 7, 2024
1 parent 772d08c commit d18bb9a
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 16 deletions.
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# httr2 (development version)

* `req_retry()` no longer treates low-level HTTP failures the same way as transient errors by default. You can return to the previous behaviour with `retry_on_error = TRUE`.
* `req_perform_iterative()` is no longer experimental.
* New `req_cookie_set()` allows you to set client side cookies (#369).
* `req_body_file()` no longer leaks a connection if the response doesn't complete succesfully (#534).
Expand Down
9 changes: 3 additions & 6 deletions R/req-perform.R
Original file line number Diff line number Diff line change
Expand Up @@ -125,19 +125,16 @@ req_perform <- function(
)
req_completed(req_prep)

if (is_error(resp)) {
if (retry_is_transient(req, resp)) {
tries <- tries + 1
delay <- retry_backoff(req, tries)
delay <- retry_after(req, resp, tries)
signal(class = "httr2_retry", tries = tries, delay = delay)
} else if (!reauth && resp_is_invalid_oauth_token(req, resp)) {
reauth <- TRUE
req <- auth_oauth_sign(req, TRUE)
req_prep <- req_prepare(req)
handle <- req_handle(req_prep)
delay <- 0
} else if (retry_is_transient(req, resp)) {
tries <- tries + 1
delay <- retry_after(req, resp, tries)
signal(class = "httr2_retry", tries = tries, delay = delay)
} else {
# done
break
Expand Down
33 changes: 23 additions & 10 deletions R/req-retries.R
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
#' `req_retry()` alters [req_perform()] so that it will automatically retry
#' in the case of failure. To activate it, you must specify either the total
#' number of requests to make with `max_tries` or the total amount of time
#' to spend with `max_seconds`. Then `req_perform()` will retry if:
#' to spend with `max_seconds`. Then `req_perform()` will retry if the error is
#' "transient", i.e. it's an HTTP error that can be resolved by waiting. By
#' default, 429 and 503 statuses are treated as transient, but if the API you
#' are wrapping has other transient status codes (or conveys transient-ness
#' with some other property of the response), you can override the default
#' with `is_transient`.
#'
#' * Either the HTTP request or HTTP response doesn't complete successfully
#' leading to an error from curl, the lower-level library that httr2 uses to
#' perform HTTP request. This occurs, for example, if your wifi is down.
#'
#' * The error is "transient", i.e. it's an HTTP error that can be resolved
#' by waiting. By default, 429 and 503 statuses are treated as transient,
#' but if the API you are wrapping has other transient status codes (or
#' conveys transient-ness with some other property of the response), you can
#' override the default with `is_transient`.
#' Additionally, if you set `retry_on_error = TRUE`, the request will retry
#' if either the HTTP request or HTTP response doesn't complete successfully
#' leading to an error from curl, the lower-level library that httr2 uses to
#' perform HTTP request. This occurs, for example, if your wifi is down.
#'
#' It's a bad idea to immediately retry a request, so `req_perform()` will
#' wait a little before trying again:
Expand Down Expand Up @@ -42,6 +42,8 @@
#' @param is_transient A predicate function that takes a single argument
#' (the response) and returns `TRUE` or `FALSE` specifying whether or not
#' the response represents a transient error.
#' @param retry_on_error Treat low-level HTTP errors as if they are transient
#' errors, and can be retried.
#' @param backoff A function that takes a single argument (the number of failed
#' attempts so far) and returns the number of seconds to wait.
#' @param after A function that takes a single argument (the response) and
Expand Down Expand Up @@ -80,16 +82,19 @@
req_retry <- function(req,
max_tries = NULL,
max_seconds = NULL,
retry_on_error = FALSE,
is_transient = NULL,
backoff = NULL,
after = NULL) {
check_request(req)
check_number_whole(max_tries, min = 2, allow_null = TRUE)
check_number_whole(max_seconds, min = 0, allow_null = TRUE)
check_bool(retry_on_error)

req_policies(req,
retry_max_tries = max_tries,
retry_max_wait = max_seconds,
retry_on_error = retry_on_error,
retry_is_transient = as_callback(is_transient, 1, "is_transient"),
retry_backoff = as_callback(backoff, 1, "backoff"),
retry_after = as_callback(after, 1, "after")
Expand All @@ -106,6 +111,10 @@ retry_max_seconds <- function(req) {
}

retry_is_transient <- function(req, resp) {
if (is_error(resp)) {
return(req$policies$retry_on_error %||% FALSE)
}

req_policy_call(req, "retry_is_transient", list(resp),
default = function(resp) resp_status(resp) %in% c(429, 503)
)
Expand All @@ -116,6 +125,10 @@ retry_backoff <- function(req, i) {
}

retry_after <- function(req, resp, i, error_call = caller_env()) {
if (is_error(resp)) {
return(retry_backoff(req, i))
}

after <- req_policy_call(req, "retry_after", list(resp), default = resp_retry_after)

# TODO: apply this idea to all callbacks
Expand Down
2 changes: 2 additions & 0 deletions R/resp-headers.R
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ resp_encoding <- function(resp) {
#' resp <- response(headers = "Retry-After: Mon, 20 Sep 2025 21:44:05 UTC")
#' resp |> resp_retry_after()
resp_retry_after <- function(resp) {
check_response(resp)

# https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
val <- resp_header(resp, "Retry-After")
if (is.null(val)) {
Expand Down
5 changes: 5 additions & 0 deletions tests/testthat/_snaps/req-retries.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@
Condition
Error in `req_retry()`:
! `max_seconds` must be a whole number or `NULL`, not the string "x".
Code
req_retry(req, retry_on_error = "x")
Condition
Error in `req_retry()`:
! `retry_on_error` must be `TRUE` or `FALSE`, not the string "x".

10 changes: 10 additions & 0 deletions tests/testthat/test-req-perform.R
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ test_that("persistent HTTP errors only get single attempt", {
expect_equal(cnd$n, 1)
})

test_that("don't retry curl errors by default", {
req <- request("frooble") %>% req_retry(max_tries = 2)
expect_error(req_perform(req), class = "httr2_failure")

# But can opt-in to it
req <- request("frooble") %>% req_retry(max_tries = 2, retry_on_error = TRUE)
cnd <- catch_cnd(req_perform(req), "httr2_retry")
expect_equal(cnd$tries, 1)
})

test_that("can retry a transient error", {
req <- local_app_request(function(req, res) {
i <- res$app$locals$i %||% 1
Expand Down
1 change: 1 addition & 0 deletions tests/testthat/test-req-retries.R
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ test_that("validates its inputs", {
expect_snapshot(error = TRUE, {
req_retry(req, max_tries = 1)
req_retry(req, max_seconds = "x")
req_retry(req, retry_on_error = "x")
})
})

Expand Down

0 comments on commit d18bb9a

Please sign in to comment.