Don't retry on internal exceptions (#1110)

* Don't retry on internal exceptions
* Make isrecoverable opt-in again

Author: Drvi

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
@@ -76,9 +76,10 @@ function retrylayer(handler)
     end
 end
 
-isrecoverable(ex) = true
-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)
@@ -92,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" : "")

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
@@ -668,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)

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