From 9c36d6920c5f3c47886c9a62d2cd238ee2bd63d8 Mon Sep 17 00:00:00 2001
From: hadley <h.wickham@gmail.com>
Date: Fri, 23 Nov 2018 09:12:02 -0600
Subject: [PATCH] Automatically terminate retries on success

Fixes #522
---
 NEWS.md                     |  4 ++++
 R/retry.R                   | 16 ++++++++++++----
 man/RETRY.Rd                |  6 +++++-
 tests/testthat/test-retry.R | 15 +++++++++++++++
 4 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 tests/testthat/test-retry.R

diff --git a/NEWS.md b/NEWS.md
index 4f9c69ad..57fcffdd 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,9 @@
 # httr 1.3.1.9000
 
+* By default, `RETRY()` now terminates on any successful request, regardless
+  of the value of `terminate_on`. To return to the previous behaviour,
+  set `terminate_on_success = FALSE` (#522).
+
 * Encoding falls back to UTF-8 if not supplied and content-type parsing
   fails (#500).
 
diff --git a/R/retry.R b/R/retry.R
index 766fab84..b6f13e9d 100644
--- a/R/retry.R
+++ b/R/retry.R
@@ -28,6 +28,9 @@
 #' @param terminate_on Optional vector of numeric HTTP status codes that if found
 #'   on the response will terminate the retry process. If \code{NULL}, will keep
 #'   retrying while \code{\link{http_error}()} is \code{TRUE} for the response.
+#' @param terminate_on_success If \code{TRUE}, the default, this will
+#'   automatically terminate when the request is successful, regardless of the
+#'   value of \code{terminate_on}.
 #' @return The last response. Note that if the request doesn't succeed after
 #'   \code{times} times this will be a failed request, i.e. you still need
 #'   to use \code{\link{stop_for_status}()}.
@@ -45,18 +48,21 @@
 RETRY <- function(verb, url = NULL, config = list(), ...,
                   body = NULL, encode = c("multipart", "form", "json", "raw"),
                   times = 3, pause_base = 1, pause_cap = 60, pause_min = 1,
-                  handle = NULL, quiet = FALSE, terminate_on = NULL) {
+                  handle = NULL, quiet = FALSE,
+                  terminate_on = NULL,
+                  terminate_on_success = TRUE) {
   stopifnot(is.numeric(times), length(times) == 1L)
   stopifnot(is.numeric(pause_base), length(pause_base) == 1L)
   stopifnot(is.numeric(pause_cap), length(pause_cap) == 1L)
   stopifnot(is.numeric(terminate_on) || is.null(terminate_on))
+  stopifnot(is.logical(terminate_on_success), length(terminate_on_success) == 1)
 
   hu <- handle_url(handle, url, ...)
   req <- request_build(verb, hu$url, body_config(body, match.arg(encode)), config, ...)
   resp <- tryCatch(request_perform(req, hu$handle$handle), error = function(e) e)
 
   i <- 1
-  while (!retry_should_terminate(i, times, resp, terminate_on)) {
+  while (!retry_should_terminate(i, times, resp, terminate_on, terminate_on_success)) {
     backoff_full_jitter(i, resp, pause_base, pause_cap, pause_min, quiet = quiet)
 
     i <- i + 1
@@ -70,8 +76,10 @@ RETRY <- function(verb, url = NULL, config = list(), ...,
   resp
 }
 
-retry_should_terminate <- function(i, times, resp, terminate_on) {
-  if (i >= times) {
+retry_should_terminate <- function(i, times, resp, terminate_on, terminate_on_success) {
+  if (terminate_on_success && !http_error(resp)) {
+    TRUE
+  } else if (i >= times) {
     TRUE
   } else if (inherits(resp, "error")) {
     FALSE
diff --git a/man/RETRY.Rd b/man/RETRY.Rd
index 3f3aaab0..4108933c 100644
--- a/man/RETRY.Rd
+++ b/man/RETRY.Rd
@@ -7,7 +7,7 @@
 RETRY(verb, url = NULL, config = list(), ..., body = NULL,
   encode = c("multipart", "form", "json", "raw"), times = 3,
   pause_base = 1, pause_cap = 60, pause_min = 1, handle = NULL,
-  quiet = FALSE, terminate_on = NULL)
+  quiet = FALSE, terminate_on = NULL, terminate_on_success = TRUE)
 }
 \arguments{
 \item{verb}{Name of verb to use.}
@@ -76,6 +76,10 @@ until the next request.}
 \item{terminate_on}{Optional vector of numeric HTTP status codes that if found
 on the response will terminate the retry process. If \code{NULL}, will keep
 retrying while \code{\link{http_error}()} is \code{TRUE} for the response.}
+
+\item{terminate_on_success}{If \code{TRUE}, the default, this will
+automatically terminate when the request is successful, regardless of the
+value of \code{terminate_on}.}
 }
 \value{
 The last response. Note that if the request doesn't succeed after
diff --git a/tests/testthat/test-retry.R b/tests/testthat/test-retry.R
new file mode 100644
index 00000000..2b744886
--- /dev/null
+++ b/tests/testthat/test-retry.R
@@ -0,0 +1,15 @@
+context("test-retry")
+
+test_that("sucessful requests terminate when terminate_on_success is true", {
+  should_terminate <- function(status_code) {
+    resp <- response(status_code = status_code)
+    retry_should_terminate(1, 10,
+      resp = resp,
+      terminate_on = 500L,
+      terminate_on_success = TRUE
+    )
+  }
+
+  expect_true(should_terminate(200L))
+  expect_false(should_terminate(400L))
+})