Clarify max_tries argument (#523)

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

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)) {

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

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,

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 <httr2_response> 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".
+

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)

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)