From 71a48cf859407455a87288a1f735068b5323bfb2 Mon Sep 17 00:00:00 2001 From: Jacob Quinn Date: Wed, 11 Jan 2023 10:17:14 -0700 Subject: [PATCH 1/3] Ensure we account for response status if present when retrying --- src/clientlayers/RetryRequest.jl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index 8fa5850ef..a225dd7fe 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/src/clientlayers/RetryRequest.jl @@ -45,7 +45,7 @@ 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) && _retry_check(s, ex, req, retry_check)) if retryattempt[] == retries req.context[:retrylimitreached] = true end @@ -67,6 +67,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) From 85ee03e2924feb4686d07e3dd9925dd7b2c97c3d Mon Sep 17 00:00:00 2001 From: Nick Robinson Date: Wed, 11 Jan 2023 20:47:51 +0000 Subject: [PATCH 2/3] WIP --- src/Messages.jl | 2 +- src/clientlayers/RetryRequest.jl | 5 ++++- test/client.jl | 6 ++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Messages.jl b/src/Messages.jl index b097e0589..83f5890f2 100644 --- a/src/Messages.jl +++ b/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, diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index a225dd7fe..e59522820 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/src/clientlayers/RetryRequest.jl @@ -45,7 +45,10 @@ function retrylayer(handler) check=(s, ex) -> begin retryattempt[] += 1 req.context[:retryattempt] = retryattempt[] - retry = (isrecoverable(ex) && 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 diff --git a/test/client.jl b/test/client.jl index f45cf3505..f8e1dc601 100644 --- a/test/client.jl +++ b/test/client.jl @@ -579,7 +579,11 @@ 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 + @show req.context str = String(resp_body) if str != "404 unexpected error" || resp.status != 404 @error "unexpected response body" str @@ -591,6 +595,8 @@ 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" + @show checked + @test checked[] >= 1 finally close(server) HTTP.ConnectionPool.closeall() From 2f13063e9254e12fec224147b49c13a214fc44c2 Mon Sep 17 00:00:00 2001 From: Jacob Quinn Date: Wed, 11 Jan 2023 15:08:08 -0700 Subject: [PATCH 3/3] cleanup --- src/clientlayers/RetryRequest.jl | 12 +++++++++--- test/client.jl | 2 -- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index e59522820..1dde5d227 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/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, diff --git a/test/client.jl b/test/client.jl index f8e1dc601..5c5e8bcc8 100644 --- a/test/client.jl +++ b/test/client.jl @@ -583,7 +583,6 @@ end @test !HTTP.retryable(404) check = (s, ex, req, resp, resp_body) -> begin checked[] += 1 - @show req.context str = String(resp_body) if str != "404 unexpected error" || resp.status != 404 @error "unexpected response body" str @@ -595,7 +594,6 @@ 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" - @show checked @test checked[] >= 1 finally close(server)