Ensure we account for response status if present when retrying (#990)

* Ensure we account for response status if present when retrying

* WIP

* cleanup

Co-authored-by: Nick Robinson <npr251@gmail.com>

src/Messages.jl

@@ -51,7 +51,7 @@ module Messages
 
 export Message, Request, Response,
        reset!, status, method, headers, uri, body, resource,
-       iserror, isredirect, retryablebody, retryable, ischunked, issafe, isidempotent,
+       iserror, isredirect, retryablebody, retryable, retrylimitreached, ischunked, issafe, isidempotent,
        header, hasheader, headercontains, setheader, defaultheader!, appendheader,
        removeheader, mkheaders, readheaders, headerscomplete,
        readchunksize,

src/clientlayers/RetryRequest.jl

@@ -16,9 +16,15 @@ Retry the request if it throws a recoverable exception.
 increasing delay is introduced between attempts to avoid exacerbating network
 congestion.
 
-Methods of `isrecoverable(e)` define which exception types lead to a retry.
-e.g. `Sockets.DNSError`, `Base.EOFError` and `HTTP.StatusError`
-(if status is `5xx`).
+By default, requests that have a retryable body, where the request wasn't written
+or is idempotent will be retries. If the request is made and a response is received
+with a status code of 403, 408, 409, 429, or 5xx, the request will be retried.
+
+`retries` controls the # of total retries that will be attempted.
+
+`retry_check` allows passing a custom retry check in the case where the default
+retry check _wouldn't_ retry, if `retry_check` returns true, then the request
+will be retried anyway.
 """
 function retrylayer(handler)
     return function(req::Request; retry::Bool=true, retries::Int=4,
@@ -45,7 +51,10 @@ function retrylayer(handler)
             check=(s, ex) -> begin
                 retryattempt[] += 1
                 req.context[:retryattempt] = retryattempt[]
-                retry = retryable(req) || retryablebody(req) && _retry_check(s, ex, req, retry_check)
+                retry = (
+                    (isrecoverable(ex) && retryable(req)) ||
+                    (retryablebody(req) && !retrylimitreached(req) && _retry_check(s, ex, req, retry_check))
+                )
                 if retryattempt[] == retries
                     req.context[:retrylimitreached] = true
                 end
@@ -67,6 +76,9 @@ function retrylayer(handler)
     end
 end
 
+isrecoverable(ex) = true
+isrecoverable(ex::StatusError) = retryable(ex.status)
+
 function _retry_check(s, ex, req, check)
     resp = req.response
     resp_body = get(req.context, :response_body, nothing)

test/client.jl

@@ -599,7 +599,10 @@ end
         shouldfail[] = false
         status[] = 404
         seekstart(req_body)
+        checked = Ref(0)
+        @test !HTTP.retryable(404)
         check = (s, ex, req, resp, resp_body) -> begin
+            checked[] += 1
             str = String(resp_body)
             if str != "404 unexpected error" || resp.status != 404
                 @error "unexpected response body" str
@@ -611,6 +614,7 @@ end
         resp = HTTP.get("http://localhost:8080/retry"; body=req_body, response_stream=res_body, retry_check=check)
         @test isok(resp)
         @test String(take!(res_body)) == "hey there sailor"
+        @test checked[] >= 1
     finally
         close(server)
         HTTP.ConnectionPool.closeall()