From dac92ac91fcb5bafd7d8489ff63261a2547e502b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 29 Aug 2024 07:45:03 -0500 Subject: [PATCH] Clarify `max_tries` argument (#523) --- NEWS.md | 1 + R/req-perform-stream.R | 2 +- R/req-perform.R | 1 + R/req-retries.R | 11 +++++++--- man/req_retry.Rd | 5 ++++- tests/testthat/_snaps/req-retries.md | 13 ++++++++++++ tests/testthat/test-req-perform.R | 30 +++++++++++++++++++++++++++- tests/testthat/test-req-retries.R | 9 +++++++++ 8 files changed, 66 insertions(+), 6 deletions(-) diff --git a/NEWS.md b/NEWS.md index 7c1df04d..6f7423aa 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # httr2 (development version) +* `req_retry()` now correctly tries `max_tries` times, rather than `max_tries - 1`. * New `req_perform_connection()` for working with streaming data. Unlike `req_perform_stream()` which uses callbacks, `req_perform_connection()` returns a regular response object with a connection as the body. It's paired with `resp_stream_bytes()`, `resp_stream_lines()`, and `resp_stream_sse()` that allows you to stream chunks as you want them. Unlike `req_perform_stream()` it supports `req_retry()` (with @jcheng5, #519). # httr2 1.0.3 diff --git a/R/req-perform-stream.R b/R/req-perform-stream.R index 0f8e6455..96fceeba 100644 --- a/R/req-perform-stream.R +++ b/R/req-perform-stream.R @@ -137,7 +137,7 @@ req_perform_connection <- function(req, max_tries <- retry_max_tries(req) deadline <- Sys.time() + retry_max_seconds(req) resp <- NULL - while (tries <= max_tries && Sys.time() < deadline) { + while (tries < max_tries && Sys.time() < deadline) { sys_sleep(delay, "for retry backoff") if (!is.null(resp)) { diff --git a/R/req-perform.R b/R/req-perform.R index 05961e30..8367a7b6 100644 --- a/R/req-perform.R +++ b/R/req-perform.R @@ -134,6 +134,7 @@ req_perform <- function( } 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 eead4f93..6c6a85b5 100644 --- a/R/req-retries.R +++ b/R/req-retries.R @@ -33,9 +33,12 @@ #' #' @inheritParams req_perform #' @param max_tries,max_seconds Cap the maximum number of attempts with -#' `max_tries` or the total elapsed time from the first request with -#' `max_seconds`. If neither option is supplied (the default), [req_perform()] -#' will not retry. +#' `max_tries` or the total elapsed time from the first request with +#' `max_seconds`. If neither option is supplied (the default), [req_perform()] +#' will not retry. +#' +#' `max_tries` is the total number of attempts make, so this should always +#' be greater than one.` #' @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. @@ -81,6 +84,8 @@ req_retry <- function(req, 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) req_policies(req, retry_max_tries = max_tries, diff --git a/man/req_retry.Rd b/man/req_retry.Rd index d9d830dd..b505aaec 100644 --- a/man/req_retry.Rd +++ b/man/req_retry.Rd @@ -19,7 +19,10 @@ req_retry( \item{max_tries, max_seconds}{Cap the maximum number of attempts with \code{max_tries} or the total elapsed time from the first request with \code{max_seconds}. If neither option is supplied (the default), \code{\link[=req_perform]{req_perform()}} -will not retry.} +will not retry. + +\code{max_tries} is the total number of attempts make, so this should always +be greater than one.`} \item{is_transient}{A predicate function that takes a single argument (the response) and returns \code{TRUE} or \code{FALSE} specifying whether or not diff --git a/tests/testthat/_snaps/req-retries.md b/tests/testthat/_snaps/req-retries.md index dfc44698..2908f96e 100644 --- a/tests/testthat/_snaps/req-retries.md +++ b/tests/testthat/_snaps/req-retries.md @@ -6,3 +6,16 @@ Error in `req_perform()`: ! The `after` callback to `req_retry()` must return a single number or NA, not a object. +# validates its inputs + + Code + req_retry(req, max_tries = 1) + Condition + Error in `req_retry()`: + ! `max_tries` must be a whole number larger than or equal to 2 or `NULL`, not the number 1. + Code + req_retry(req, max_seconds = "x") + Condition + Error in `req_retry()`: + ! `max_seconds` must be a whole number or `NULL`, not the string "x". + diff --git a/tests/testthat/test-req-perform.R b/tests/testthat/test-req-perform.R index a3fceca6..f6385f7c 100644 --- a/tests/testthat/test-req-perform.R +++ b/tests/testthat/test-req-perform.R @@ -54,6 +54,34 @@ test_that("persistent HTTP errors only get single attempt", { expect_equal(cnd$n, 1) }) +test_that("can retry a transient error", { + skip_on_covr() + app <- webfakes::new_app() + + app$get("/retry", function(req, res) { + i <- res$app$locals$i %||% 1 + if (i == 1) { + res$app$locals$i <- 2 + res$ + set_status(429)$ + set_header("retry-after", 0)$ + send_json(list(status = "waiting")) + } else { + res$send_json(list(status = "done")) + } + }) + + server <- webfakes::local_app_process(app) + req <- request(server$url("/retry")) %>% + req_retry(max_tries = 2) + + cnd <- catch_cnd(resp <- req_perform(req), "httr2_retry") + expect_s3_class(cnd, "httr2_retry") + expect_equal(cnd$tries, 1) + expect_equal(cnd$delay, 0) +}) + + test_that("repeated transient errors still fail", { req <- request_test("/status/:status", status = 429) %>% req_retry(max_tries = 3, backoff = ~0) @@ -87,7 +115,7 @@ test_that("can retrieve last request and response", { }) test_that("can last response is NULL if it fails", { - req <- request("frooble") + req <- request("frooble") %>% req_timeout(0.1) 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 3f255445..7f9b002f 100644 --- a/tests/testthat/test-req-retries.R +++ b/tests/testthat/test-req-retries.R @@ -66,6 +66,15 @@ test_that("useful message if `after` wrong", { expect_snapshot(req_perform(req), error = TRUE) }) +test_that("validates its inputs", { + req <- new_request("http://example.com") + + expect_snapshot(error = TRUE, { + req_retry(req, max_tries = 1) + req_retry(req, max_seconds = "x") + }) +}) + test_that("is_number_or_na implemented correctly", { expect_equal(is_number_or_na(1), TRUE) expect_equal(is_number_or_na(NA_real_), TRUE)