Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug logging messages on retry #549

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ XMLDict = "0.3, 0.4"
julia = "1.6"

[extras]
Logging = "56ddb016-857b-54e1-b83d-db4d58db5568"
Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f"
StableRNGs = "860ef19b-820b-49d6-a774-d7a799459cd3"
Suppressor = "fd094767-a336-5f1f-9728-57cf17d0bbfb"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"

[targets]
test = ["Pkg", "Suppressor", "StableRNGs", "Test", "UUIDs"]
test = ["Pkg", "Logging", "StableRNGs", "Suppressor", "Test", "UUIDs"]
13 changes: 13 additions & 0 deletions src/utilities/downloads_backend.jl
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,19 @@ function _http_request(backend::DownloadsBackend, request::Request, response_str
return nothing
end

check = (s, e) -> begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
check = (s, e) -> begin
check =
(s, e) -> begin

if isa(e, HTTP.StatusError) && AWS._http_status(e) >= 500
@debug "AWS.jl Downloads inner retry for status >= 500" retry=true reason="status >= 500" status=_http_status(e) exception=e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@debug "AWS.jl Downloads inner retry for status >= 500" retry=true reason="status >= 500" status=_http_status(e) exception=e
@debug "AWS.jl Downloads inner retry for status >= 500" retry = true reason = "status >= 500" status = _http_status(
e
) exception = e

return true
elseif isa(e, Downloads.RequestError)
@debug "AWS.jl Downloads inner retry for Downloads.RequestError" retry=true reason="Downloads.RequestError" exception=e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@debug "AWS.jl Downloads inner retry for Downloads.RequestError" retry=true reason="Downloads.RequestError" exception=e
@debug "AWS.jl Downloads inner retry for Downloads.RequestError" retry =
true reason = "Downloads.RequestError" exception = e

return true
else
@debug "AWS.jl Downloads inner retry declined" retry=false reason="Exception passed onwards" exception=e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@debug "AWS.jl Downloads inner retry declined" retry=false reason="Exception passed onwards" exception=e
@debug "AWS.jl Downloads inner retry declined" retry = false reason = "Exception passed onwards" exception =
e

return false
end
end

try
retry(get_response; check=check, delays=delays)()
finally
Expand Down
64 changes: 51 additions & 13 deletions src/utilities/request.jl
Original file line number Diff line number Diff line change
Expand Up @@ -147,52 +147,73 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers
if e isa HTTP.StatusError
e = AWSException(e, stream)
rethrow(e)
elseif !(e isa AWSException)
@debug "AWS.jl declined to retry non-AWSException" retry = false reason = "Non-AWSException" exception =
e
return false
end
rethrow()
end
end

check = function (s, e)
check = (s, e) -> begin
# Pass on non-AWS exceptions.
if !(e isa AWSException)
@debug "AWS.jl declined to retry non-AWSException" retry=false reason="Non-AWSException" exception=e
return false
end
Comment on lines +157 to 162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
check = (s, e) -> begin
# Pass on non-AWS exceptions.
if !(e isa AWSException)
@debug "AWS.jl declined to retry non-AWSException" retry=false reason="Non-AWSException" exception=e
return false
end
check =
(s, e) -> begin
# Pass on non-AWS exceptions.
if !(e isa AWSException)
@debug "AWS.jl declined to retry non-AWSException" retry = false reason = "Non-AWSException" exception =
e
return false
end


occursin("Signature expired", e.message) && return true
if occursin("Signature expired", e.message)
@debug "AWS.jl retry for signature expired" retry=true reason="Signature expired" exception=e
return true
end
Comment on lines +164 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
if occursin("Signature expired", e.message)
@debug "AWS.jl retry for signature expired" retry=true reason="Signature expired" exception=e
return true
end
if occursin("Signature expired", e.message)
@debug "AWS.jl retry for signature expired" retry = true reason = "Signature expired" exception =
e
return true
end


# Handle ExpiredToken...
# https://github.com/aws/aws-sdk-go/blob/v1.31.5/aws/request/retryer.go#L98
if e isa AWSException && e.code in EXPIRED_ERROR_CODES
@debug "AWS.jl retry for expired credentials" retry=true reason="Credentials expired" exception=e
check_credentials(credentials(aws); force_refresh=true)
return true
end
Comment on lines 169 to 175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
# Handle ExpiredToken...
# https://github.com/aws/aws-sdk-go/blob/v1.31.5/aws/request/retryer.go#L98
if e isa AWSException && e.code in EXPIRED_ERROR_CODES
@debug "AWS.jl retry for expired credentials" retry=true reason="Credentials expired" exception=e
check_credentials(credentials(aws); force_refresh=true)
return true
end
# Handle ExpiredToken...
# https://github.com/aws/aws-sdk-go/blob/v1.31.5/aws/request/retryer.go#L98
if e isa AWSException && e.code in EXPIRED_ERROR_CODES
@debug "AWS.jl retry for expired credentials" retry = true reason = "Credentials expired" exception =
e
check_credentials(credentials(aws); force_refresh=true)
return true
end


# Throttle handling
# https://github.com/boto/botocore/blob/1.16.17/botocore/data/_retry.json
# https://docs.aws.amazon.com/general/latest/gr/api-retries.html
if _http_status(e.cause) == TOO_MANY_REQUESTS || e.code in THROTTLING_ERROR_CODES
if _http_status(e.cause) == TOO_MANY_REQUESTS
@debug "AWS.jl retry too many requests" retry=true reason="too many requests" exception=e
return true
elseif e.code in THROTTLING_ERROR_CODES
@debug "AWS.jl retry for throttling" retry=true reason="throttled" exception=e
return true
end
Comment on lines 177 to 186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
# Throttle handling
# https://github.com/boto/botocore/blob/1.16.17/botocore/data/_retry.json
# https://docs.aws.amazon.com/general/latest/gr/api-retries.html
if _http_status(e.cause) == TOO_MANY_REQUESTS || e.code in THROTTLING_ERROR_CODES
if _http_status(e.cause) == TOO_MANY_REQUESTS
@debug "AWS.jl retry too many requests" retry=true reason="too many requests" exception=e
return true
elseif e.code in THROTTLING_ERROR_CODES
@debug "AWS.jl retry for throttling" retry=true reason="throttled" exception=e
return true
end
# Throttle handling
# https://github.com/boto/botocore/blob/1.16.17/botocore/data/_retry.json
# https://docs.aws.amazon.com/general/latest/gr/api-retries.html
if _http_status(e.cause) == TOO_MANY_REQUESTS
@debug "AWS.jl retry too many requests" retry = true reason = "too many requests" exception =
e
return true
elseif e.code in THROTTLING_ERROR_CODES
@debug "AWS.jl retry for throttling" retry = true reason = "throttled" exception =
e
return true
end


# Handle BadDigest error and CRC32 check sum failure
if _header(e.cause, "crc32body") == "x-amz-crc32" ||
e.code in ("BadDigest", "RequestTimeout", "RequestTimeoutException")
if _header(e.cause, "crc32body") == "x-amz-crc32"
@debug "AWS.jl retry for check sum failure" retry=true reason="Check sum failure" exception=e
return true
end
Comment on lines 188 to +192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
# Handle BadDigest error and CRC32 check sum failure
if _header(e.cause, "crc32body") == "x-amz-crc32" ||
e.code in ("BadDigest", "RequestTimeout", "RequestTimeoutException")
if _header(e.cause, "crc32body") == "x-amz-crc32"
@debug "AWS.jl retry for check sum failure" retry=true reason="Check sum failure" exception=e
return true
end
# Handle BadDigest error and CRC32 check sum failure
if _header(e.cause, "crc32body") == "x-amz-crc32"
@debug "AWS.jl retry for check sum failure" retry = true reason = "Check sum failure" exception =
e
return true
end


if e.code in ("BadDigest", "RequestTimeout", "RequestTimeoutException")
@debug "AWS.jl retry for $(e.code)" retry=true reason="$(e.code)" exception=e
return true
end
Comment on lines +194 to 197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
if e.code in ("BadDigest", "RequestTimeout", "RequestTimeoutException")
@debug "AWS.jl retry for $(e.code)" retry=true reason="$(e.code)" exception=e
return true
end
if e.code in ("BadDigest", "RequestTimeout", "RequestTimeoutException")
@debug "AWS.jl retry for $(e.code)" retry = true reason = "$(e.code)" exception =
e
return true
end


if occursin("Missing Authentication Token", e.message) &&
aws.credentials === nothing
@debug "AWS.jl declined to retry request without credentials" retry=false reason="No credentials" exception=e
Comment on lines 199 to +201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
if occursin("Missing Authentication Token", e.message) &&
aws.credentials === nothing
@debug "AWS.jl declined to retry request without credentials" retry=false reason="No credentials" exception=e
if occursin("Missing Authentication Token", e.message) &&
aws.credentials === nothing
@debug "AWS.jl declined to retry request without credentials" retry = false reason = "No credentials" exception = e


return throw(
NoCredentials(
"You're attempting to perform a request without credentials set."
"You're attempting to perform a request without credentials set.",
),
)
end
Comment on lines 203 to 208
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
return throw(
NoCredentials(
"You're attempting to perform a request without credentials set."
"You're attempting to perform a request without credentials set.",
),
)
end
return throw(
NoCredentials(
"You're attempting to perform a request without credentials set.",
),
)
end


@debug "AWS.jl declined to retry uncaught error" retry=false reason="Error unhandled" exception=e
return false
end
Comment on lines +210 to 212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@debug "AWS.jl declined to retry uncaught error" retry=false reason="Error unhandled" exception=e
return false
end
@debug "AWS.jl declined to retry uncaught error" retry = false reason = "Error unhandled" exception =
e
return false
end


delays = AWSExponentialBackoff(; max_attempts=max_attempts(aws))

retry(upgrade_error(get_response); check=check, delays=delays)()

if request.use_response_type
Expand Down Expand Up @@ -234,14 +255,31 @@ function _http_request(http_backend::HTTPBackend, request::Request, response_str
return nothing
end

check = function (s, e)
return isa(e, HTTP.ConnectError) ||
isa(e, HTTP.RequestError) ||
(isa(e, HTTP.StatusError) && _http_status(e) >= 500)
end

delays = AWSExponentialBackoff(; max_attempts=4)

check = (s, e) -> begin
# `Base.IOError` is needed because HTTP.jl can often have errors that aren't
# caught and wrapped in an `HTTP.IOError`
# https://github.com/JuliaWeb/HTTP.jl/issues/382
errors = (Sockets.DNSError, HTTP.ParseError, HTTP.IOError, Base.IOError)
for error in errors
if isa(e, error)
@debug "AWS.jl HTTP inner retry for $error" retry = true reason = "$error" exception =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
check = (s, e) -> begin
# `Base.IOError` is needed because HTTP.jl can often have errors that aren't
# caught and wrapped in an `HTTP.IOError`
# https://github.com/JuliaWeb/HTTP.jl/issues/382
errors = (Sockets.DNSError, HTTP.ParseError, HTTP.IOError, Base.IOError)
for error in errors
if isa(e, error)
@debug "AWS.jl HTTP inner retry for $error" retry = true reason = "$error" exception =
check =
(s, e) -> begin
# `Base.IOError` is needed because HTTP.jl can often have errors that aren't
# caught and wrapped in an `HTTP.IOError`
# https://github.com/JuliaWeb/HTTP.jl/issues/382
errors = (Sockets.DNSError, HTTP.ParseError, HTTP.IOError, Base.IOError)
for error in errors
if isa(e, error)
@debug "AWS.jl HTTP inner retry for $error" retry = true reason = "$error" exception =
e
return true
end
end
if (isa(e, HTTP.StatusError) && _http_status(e) >= 500)
@debug "AWS.jl HTTP inner retry for status >= 500" retry = true reason = "status >= 500" status = AWS._http_status(

e
return true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
return true
) exception = e
return true

end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
end
@debug "AWS.jl HTTP inner retry declined" retry = false reason = "Exception passed onwards" exception =
e
return false
end

if (isa(e, HTTP.StatusError) && _http_status(e) >= 500)
@debug "AWS.jl HTTP inner retry for status >= 500" retry = true reason = "status >= 500" status = AWS._http_status(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
end
if (isa(e, HTTP.StatusError) && _http_status(e) >= 500)
@debug "AWS.jl HTTP inner retry for status >= 500" retry = true reason = "status >= 500" status = AWS._http_status(
@debug "AWS.jl HTTP inner retry declined" retry = false reason = "Exception passed onwards" exception =

e
) exception = e
return true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
) exception = e
return true
return false

end
@debug "AWS.jl HTTP inner retry declined" retry = false reason = "Exception passed onwards" exception =
e
return false
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
@debug "AWS.jl HTTP inner retry declined" retry = false reason = "Exception passed onwards" exception =
e
return false
end


try
retry(get_response; check=check, delays=delays)()
finally
Expand Down
46 changes: 35 additions & 11 deletions test/issues.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

BUCKET_NAME = "aws-jl-test-issues---" * _now_formatted()

# We try to be insensitive to the existence of other logging messages, so we test
# by counting the number of messages that meet some criteria
function log_is_retry(successful)
return record ->
record.level == Logging.Debug &&
get(record.kwargs, :retry, nothing) == successful
Comment on lines +9 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
record.level == Logging.Debug &&
get(record.kwargs, :retry, nothing) == successful
record.level == Logging.Debug && get(record.kwargs, :retry, nothing) == successful

end

try
S3.create_bucket(BUCKET_NAME)

Expand Down Expand Up @@ -202,12 +210,20 @@ try
config = AWSConfig(; creds=nothing)

@testset "Fail 2 attempts then succeed" begin
apply(_incomplete_patch(; data=data, num_attempts_to_fail=2)) do
retrieved = S3.get_object(bucket, key; aws_config=config)
logger = Test.TestLogger(; min_level=Logging.Debug)
with_logger(logger) do
apply(_incomplete_patch(; data=data, num_attempts_to_fail=2)) do
retrieved = S3.get_object(bucket, key; aws_config=config)

@test length(retrieved) == n
@test retrieved == data
@test length(retrieved) == n
@test retrieved == data
end
end
logs = logger.logs
# Two successful retries
@test count(log_is_retry(true), logs) == 2
# No unsuccessful ones
@test count(log_is_retry(false), logs) == 0
end

@testset "Fail all 4 attempts then throw" begin
Expand All @@ -218,15 +234,23 @@ try
end
io = IOBuffer()

apply(_incomplete_patch(; data=data, num_attempts_to_fail=4)) do
params = Dict("response_stream" => io)
@test_throws err_t S3.get_object(bucket, key, params; aws_config=config)
logger = Test.TestLogger(; min_level=Logging.Debug)
with_logger(logger) do
apply(_incomplete_patch(; data=data, num_attempts_to_fail=4)) do
params = Dict("response_stream" => io)
@test_throws err_t S3.get_object(bucket, key, params; aws_config=config)

seekstart(io)
retrieved = read(io)
@test length(retrieved) == n - 1
@test retrieved == data[1:(n - 1)]
seekstart(io)
retrieved = read(io)
@test length(retrieved) == n - 1
@test retrieved == data[1:(n - 1)]
end
end
logs = logger.logs
# Three successful retries - from the inner retry loop
@test count(log_is_retry(true), logs) == 3
# One unsuccessful one - from the outer loop where we pass it on
@test count(log_is_retry(false), logs) == 1
end
end

Expand Down
5 changes: 3 additions & 2 deletions test/runtests.jl
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,17 @@ using GitHub
using HTTP
using IniFile: Inifile
using JSON
using OrderedCollections: LittleDict, OrderedDict
using Logging
using MbedTLS: digest, MD_SHA256, MD_MD5
using Mocking
using OrderedCollections: LittleDict, OrderedDict
using Pkg
using Random
using StableRNGs
using Suppressor
using Test
using UUIDs
using XMLDict
using StableRNGs

Mocking.activate()

Expand Down