From 54601085451e32b5b586ae261d1b212543e41810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Drvo=C5=A1t=C4=9Bp?= Date: Thu, 14 Sep 2023 14:25:02 +0200 Subject: [PATCH 1/3] Don't retry on internal exceptions --- src/clientlayers/RetryRequest.jl | 3 ++ test/client.jl | 47 ++++++++++++++++++++++++++++++++ test/resources/TestRequest.jl | 22 +++++++++++++++ 3 files changed, 72 insertions(+) diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index f4e8d1c19..9faec8111 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/src/clientlayers/RetryRequest.jl @@ -77,6 +77,9 @@ function retrylayer(handler) end isrecoverable(ex) = true +isrecoverable(ex::ArgumentError) = false +isrecoverable(ex::ErrorException) = false +isrecoverable(ex::RequestError) = isrecoverable(ex.error) isrecoverable(ex::CapturedException) = isrecoverable(ex.ex) isrecoverable(ex::ConnectError) = isrecoverable(ex.error) # Treat all DNS errors except `EAI_AGAIN`` as non-recoverable diff --git a/test/client.jl b/test/client.jl index f886db705..c33cc58bb 100644 --- a/test/client.jl +++ b/test/client.jl @@ -645,6 +645,53 @@ end end end +@testset "Don't retry on internal exceptions" begin + kws = (retry_delays = [1, 2, 3], retries=3) # ~ 6 secs + max_wait = 3 + + function test_finish_within(f, secs) + timedout = Ref(false) + t = Timer((t)->(timedout[] = true), secs) + try + f() + finally + close(t) + end + @test !timedout[] + end + + expected = ErrorException("request") + test_finish_within(max_wait) do + @test_throws expected ErrorRequest.get("https://$httpbin/ip"; request_exception=expected, kws...) + end + expected = ArgumentError("request") + test_finish_within(max_wait) do + @test_throws expected ErrorRequest.get("https://$httpbin/ip"; request_exception=expected, kws...) + end + + test_finish_within(max_wait) do + expected = ErrorException("stream") + e = try + ErrorRequest.get("https://$httpbin/ip"; stream_exception=expected, kws...) + catch e + e + end + @assert e isa HTTP.RequestError + @test e.error == expected + end + + test_finish_within(max_wait) do + expected = ArgumentError("stream") + e = try + ErrorRequest.get("https://$httpbin/ip"; stream_exception=expected, kws...) + catch e + e + end + @assert e isa HTTP.RequestError + @test e.error == expected + end +end + @testset "Retry with ConnectError" begin mktemp() do path, io redirect_stdout(io) do diff --git a/test/resources/TestRequest.jl b/test/resources/TestRequest.jl index 98f5e65b0..01caa71b8 100644 --- a/test/resources/TestRequest.jl +++ b/test/resources/TestRequest.jl @@ -55,3 +55,25 @@ end HTTP.@client (first=[testouterrequestlayer], last=[testinnerrequestlayer]) (first=[testouterstreamlayer], last=[testinnerstreamlayer]) end + +module ErrorRequest + +using HTTP + +function throwingrequestlayer(handler) + return function(req; request_exception=nothing, kw...) + !isnothing(request_exception) && throw(request_exception) + return handler(req; kw...) + end +end + +function throwingstreamlayer(handler) + return function(stream; stream_exception=nothing, kw...) + !isnothing(stream_exception) && throw(stream_exception) + return handler(stream; kw...) + end +end + +HTTP.@client (throwingrequestlayer,) (throwingstreamlayer,) + +end From f976002043ec5b99014ae9b3d477c23ba831c8ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Drvo=C5=A1t=C4=9Bp?= Date: Fri, 15 Sep 2023 12:47:54 +0200 Subject: [PATCH 2/3] Make isrecoverable opt-in again --- src/clientlayers/RetryRequest.jl | 18 +++++++------- test/client.jl | 41 +++++++++++++++++++++++++++----- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index 9faec8111..efa39ac18 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/src/clientlayers/RetryRequest.jl @@ -1,6 +1,6 @@ module RetryRequest -using Sockets, LoggingExtras, MbedTLS, OpenSSL +using Sockets, LoggingExtras, MbedTLS, OpenSSL, ExceptionUnwrapping using ..IOExtras, ..Messages, ..Strings, ..ExceptionRequest, ..Exceptions export retrylayer @@ -67,7 +67,7 @@ function retrylayer(handler) mark(req.body) end else - @debugv 1 "🚷 No Retry: $(no_retry_reason(ex, req))" + @debugv 1 "🚷 No Retry: $(no_retry_reason(unwrapped_ex, req))" end return s, retry end @@ -76,12 +76,10 @@ function retrylayer(handler) end end -isrecoverable(ex) = true -isrecoverable(ex::ArgumentError) = false -isrecoverable(ex::ErrorException) = false -isrecoverable(ex::RequestError) = isrecoverable(ex.error) -isrecoverable(ex::CapturedException) = isrecoverable(ex.ex) -isrecoverable(ex::ConnectError) = isrecoverable(ex.error) +isrecoverable(ex) = is_wrapped_exception(ex) ? isrecoverable(unwrap_exception(ex)) : false +isrecoverable(::Union{Base.EOFError, Base.IOError, MbedTLS.MbedException, OpenSSL.OpenSSLError}) = true +isrecoverable(ex::ArgumentError) = ex.msg == "stream is closed or unusable" +isrecoverable(ex::CompositeException) = all(isrecoverable, ex.exceptions) # Treat all DNS errors except `EAI_AGAIN`` as non-recoverable # Ref: https://github.com/JuliaLang/julia/blob/ec8df3da3597d0acd503ff85ac84a5f8f73f625b/stdlib/Sockets/src/addrinfo.jl#L108-L112 isrecoverable(ex::Sockets.DNSError) = (ex.code == Base.UV_EAI_AGAIN) @@ -95,9 +93,11 @@ end function no_retry_reason(ex, req) buf = IOBuffer() + unwrapped_ex = unwrap_exception(ex) show(IOContext(buf, :compact => true), req) print(buf, ", ", - ex isa StatusError ? "HTTP $(ex.status): " : + unwrapped_ex isa StatusError ? "HTTP $(ex.status): " : + !isrecoverable(unwrapped_ex) ? "unrecoverable exception: " : !isbytes(req.body) ? "request streamed, " : "", !isbytes(req.response.body) ? "response streamed, " : "", !isidempotent(req) ? "$(req.method) non-idempotent" : "") diff --git a/test/client.jl b/test/client.jl index c33cc58bb..e9c60e7c3 100644 --- a/test/client.jl +++ b/test/client.jl @@ -715,12 +715,41 @@ end end # isrecoverable tests - @test HTTP.RetryRequest.isrecoverable(nothing) - @test HTTP.RetryRequest.isrecoverable(ErrorException("")) - @test HTTP.RetryRequest.isrecoverable(Sockets.DNSError("localhost", Base.UV_EAI_AGAIN)) - @test !HTTP.RetryRequest.isrecoverable(Sockets.DNSError("localhost", Base.UV_EAI_NONAME)) - @test HTTP.RetryRequest.isrecoverable(HTTP.Exceptions.ConnectError("http://localhost", Sockets.DNSError("localhost", Base.UV_EAI_AGAIN))) - @test !HTTP.RetryRequest.isrecoverable(HTTP.Exceptions.ConnectError("http://localhost", Sockets.DNSError("localhost", Base.UV_EAI_NONAME))) + @test !HTTP.RetryRequest.isrecoverable(nothing) + + @test !HTTP.RetryRequest.isrecoverable(ErrorException("")) + @test !HTTP.RetryRequest.isrecoverable(ArgumentError("yikes")) + @test HTTP.RetryRequest.isrecoverable(ArgumentError("stream is closed or unusable")) + + @test HTTP.RetryRequest.isrecoverable(HTTP.RequestError(nothing, ArgumentError("stream is closed or unusable"))) + @test !HTTP.RetryRequest.isrecoverable(HTTP.RequestError(nothing, ArgumentError("yikes"))) + + @test HTTP.RetryRequest.isrecoverable(CapturedException(ArgumentError("stream is closed or unusable"), Any[])) + + recoverable_dns_error = Sockets.DNSError("localhost", Base.UV_EAI_AGAIN) + unrecoverable_dns_error = Sockets.DNSError("localhost", Base.UV_EAI_NONAME) + @test HTTP.RetryRequest.isrecoverable(recoverable_dns_error) + @test !HTTP.RetryRequest.isrecoverable(unrecoverable_dns_error) + @test HTTP.RetryRequest.isrecoverable(HTTP.Exceptions.ConnectError("http://localhost", recoverable_dns_error)) + @test !HTTP.RetryRequest.isrecoverable(HTTP.Exceptions.ConnectError("http://localhost", unrecoverable_dns_error)) + @test HTTP.RetryRequest.isrecoverable(CompositeException([ + recoverable_dns_error, + HTTP.Exceptions.ConnectError("http://localhost", recoverable_dns_error), + ])) + @test HTTP.RetryRequest.isrecoverable(CompositeException([ + recoverable_dns_error, + HTTP.Exceptions.ConnectError("http://localhost", recoverable_dns_error), + CompositeException([recoverable_dns_error]) + ])) + @test !HTTP.RetryRequest.isrecoverable(CompositeException([ + recoverable_dns_error, + HTTP.Exceptions.ConnectError("http://localhost", unrecoverable_dns_error), + ])) + @test !HTTP.RetryRequest.isrecoverable(CompositeException([ + recoverable_dns_error, + HTTP.Exceptions.ConnectError("http://localhost", recoverable_dns_error), + CompositeException([unrecoverable_dns_error]) + ])) end findnewline(bytes) = something(findfirst(==(UInt8('\n')), bytes), 0) From 2997de92aaa1c7c647fac9243ac356dff7384c87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Drvo=C5=A1t=C4=9Bp?= Date: Fri, 15 Sep 2023 13:10:54 +0200 Subject: [PATCH 3/3] Fix typo --- src/clientlayers/RetryRequest.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clientlayers/RetryRequest.jl b/src/clientlayers/RetryRequest.jl index efa39ac18..001c22128 100644 --- a/src/clientlayers/RetryRequest.jl +++ b/src/clientlayers/RetryRequest.jl @@ -67,7 +67,7 @@ function retrylayer(handler) mark(req.body) end else - @debugv 1 "🚷 No Retry: $(no_retry_reason(unwrapped_ex, req))" + @debugv 1 "🚷 No Retry: $(no_retry_reason(ex, req))" end return s, retry end