diff --git a/NEWS.md b/NEWS.md index f1b834fa..2219be76 100644 --- a/NEWS.md +++ b/NEWS.md @@ -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). diff --git a/R/req-perform.R b/R/req-perform.R index cab28baa..3b950eb1 100644 --- a/R/req-perform.R +++ b/R/req-perform.R @@ -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 diff --git a/R/req-retries.R b/R/req-retries.R index 6c6a85b5..202391a2 100644 --- a/R/req-retries.R +++ b/R/req-retries.R @@ -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_failure = 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: @@ -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_failure Treat low-level failures 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 @@ -80,16 +82,19 @@ req_retry <- function(req, max_tries = NULL, max_seconds = NULL, + retry_on_failure = 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_failure) req_policies(req, retry_max_tries = max_tries, retry_max_wait = max_seconds, + retry_on_failure = retry_on_failure, retry_is_transient = as_callback(is_transient, 1, "is_transient"), retry_backoff = as_callback(backoff, 1, "backoff"), retry_after = as_callback(after, 1, "after") @@ -106,6 +111,10 @@ retry_max_seconds <- function(req) { } retry_is_transient <- function(req, resp) { + if (is_error(resp)) { + return(req$policies$retry_on_failure %||% FALSE) + } + req_policy_call(req, "retry_is_transient", list(resp), default = function(resp) resp_status(resp) %in% c(429, 503) ) @@ -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 diff --git a/R/resp-headers.R b/R/resp-headers.R index bb5a6c2a..f90fc3c7 100644 --- a/R/resp-headers.R +++ b/R/resp-headers.R @@ -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)) { diff --git a/man/req_retry.Rd b/man/req_retry.Rd index f91a19d1..9c3d5044 100644 --- a/man/req_retry.Rd +++ b/man/req_retry.Rd @@ -8,6 +8,7 @@ req_retry( req, max_tries = NULL, max_seconds = NULL, + retry_on_failure = FALSE, is_transient = NULL, backoff = NULL, after = NULL @@ -24,6 +25,9 @@ will not retry. \code{max_tries} is the total number of attempts make, so this should always be greater than one.`} +\item{retry_on_failure}{Treat low-level failures as if they are +transient errors, and can be retried.} + \item{is_transient}{A predicate function that takes a single argument (the response) and returns \code{TRUE} or \code{FALSE} specifying whether or not the response represents a transient error.} @@ -43,17 +47,17 @@ A modified HTTP \link{request}. \code{req_retry()} alters \code{\link[=req_perform]{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 \code{max_tries} or the total amount of time -to spend with \code{max_seconds}. Then \code{req_perform()} will retry if: -\itemize{ -\item Either the HTTP request or HTTP response doesn't complete successfully +to spend with \code{max_seconds}. Then \code{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 \code{is_transient}. + +Additionally, if you set \code{retry_on_failure = 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. -\item 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 \code{is_transient}. -} It's a bad idea to immediately retry a request, so \code{req_perform()} will wait a little before trying again: diff --git a/tests/testthat/_snaps/req-retries.md b/tests/testthat/_snaps/req-retries.md index 2908f96e..a44fc28d 100644 --- a/tests/testthat/_snaps/req-retries.md +++ b/tests/testthat/_snaps/req-retries.md @@ -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_failure = "x") + Condition + Error in `req_retry()`: + ! `retry_on_failure` must be `TRUE` or `FALSE`, not the string "x". diff --git a/tests/testthat/test-req-perform.R b/tests/testthat/test-req-perform.R index 75060208..8db68975 100644 --- a/tests/testthat/test-req-perform.R +++ b/tests/testthat/test-req-perform.R @@ -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("") %>% req_retry(max_tries = 2) + expect_error(req_perform(req), class = "httr2_failure") + + # But can opt-in to it + req <- request("") %>% req_retry(max_tries = 2, retry_on_failure = 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 @@ -163,7 +173,7 @@ test_that("can retrieve last request and response", { }) test_that("can last response is NULL if it fails", { - req <- request("frooble") %>% req_timeout(0.1) + req <- request("") try(req_perform(req), silent = TRUE) expect_equal(last_request(), req) diff --git a/tests/testthat/test-req-retries.R b/tests/testthat/test-req-retries.R index 7f9b002f..c59b3dca 100644 --- a/tests/testthat/test-req-retries.R +++ b/tests/testthat/test-req-retries.R @@ -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_failure = "x") }) })