From bc2df9e4f19f9788a403bdcf90dc2a5e062d3bad Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 15:03:52 +0200 Subject: [PATCH 01/14] New Downloader for transient retries --- src/utilities/downloads_backend.jl | 31 ++++++++++++++++++++---------- src/utilities/request.jl | 23 +++++++++++++++++++--- src/utilities/utilities.jl | 17 ++++++++++++++++ 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 88e1b87719..219e288918 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -6,12 +6,11 @@ using HTTP.MessageRequest: body_was_streamed This backend uses the Downloads.jl stdlib to use libcurl as an HTTP client to connect to the AWS REST API. -It has one field, +It has two fields, -- `downloader::Union{Nothing,Downloads.Downloader}` - -which is the `Downloads.Downloader` to use. If set to `nothing`, the default, -then a global downloader object will be used. +- `downloader::Union{Nothing,Downloads.Downloader}`: if `nothing`, use a global Downloader object. Otherwise, uses the given Downloader. +- `create_new_downloader::Any`: a zero-argument function which returns a new Downloader object to use. + Defaults to creating a new global downloader. This is called when a transient error occurs. Downloads.jl tends to perform better under concurrent operation than HTTP.jl, particularly with `@async` / `asyncmap`. As of March 2022, threading (e.g. `@spawn` or `@threads`) with Downloads.jl is broken on all releases of Julia ([Downloads.jl#110](https://github.com/JuliaLang/Downloads.jl/issues/110)), and there are still reported issues on the upcoming @@ -19,9 +18,11 @@ particularly with `@async` / `asyncmap`. As of March 2022, threading (e.g. `@spa """ struct DownloadsBackend <: AWS.AbstractBackend downloader::Union{Nothing,Downloads.Downloader} + create_new_downloader::Any end -DownloadsBackend() = DownloadsBackend(nothing) +DownloadsBackend() = DownloadsBackend(nothing, () -> get_downloader(; fresh=true)) +DownloadsBackend(D::Downloader) = DownloadsBackend(D, () -> get_downloader(; fresh=true)) const AWS_DOWNLOADER = Ref{Union{Nothing,Downloader}}(nothing) const AWS_DOWNLOAD_LOCK = ReentrantLock() @@ -31,13 +32,14 @@ const AWS_DOWNLOAD_LOCK = ReentrantLock() # because we add a hook to avoid redirects in order to try to match the HTTPBackend's # implementation, and we don't want to mutate the global downloader from Downloads.jl. # https://github.com/JuliaLang/Downloads.jl/blob/84e948c02b8a0625552a764bf90f7d2ee97c949c/src/Downloads.jl#L293-L301 -function get_downloader(downloader=nothing) +function get_downloader(; fresh=false) + downloader = nothing lock(AWS_DOWNLOAD_LOCK) do yield() # let other downloads finish downloader isa Downloader && return nothing while true downloader = AWS_DOWNLOADER[] - downloader isa Downloader && return nothing + !fresh && downloader isa Downloader && return nothing D = Downloader() D.easy_hook = (easy, info) -> Curl.setopt(easy, Curl.CURLOPT_FOLLOWLOCATION, false) @@ -57,14 +59,18 @@ function read_body(x::IO) return read(x) end -function _http_request(backend::DownloadsBackend, request::Request, response_stream::IO) +function _http_request(backend::DownloadsBackend, request::Request, response_stream::IO; transient_retry=false) # HTTP.jl sets this header automatically. request.headers["Content-Length"] = string(body_length(request.content)) # We pass an `input` only when we have content we wish to send. input = !isempty(request.content) ? IOBuffer(request.content) : nothing - downloader = @something(backend.downloader, get_downloader()) + if transient_retry + downloader = backend.create_new_downloader() + else + downloader = @something(backend.downloader, get_downloader()) + end # set the hook so that we don't follow redirects. Only # need to do this on per-request downloaders, because we @@ -108,6 +114,11 @@ function _http_request(backend::DownloadsBackend, request::Request, response_str check = (s, e) -> begin + if is_transient_error(e) + # We want a new one, ref https://github.com/JuliaCloud/AWS.jl/issues/552 + downloader = backend.create_new_downloader() + return true + end return (isa(e, HTTP.StatusError) && AWS._http_status(e) >= 500) || isa(e, Downloads.RequestError) end diff --git a/src/utilities/request.jl b/src/utilities/request.jl index c51d7cf33d..1a7843bfdd 100644 --- a/src/utilities/request.jl +++ b/src/utilities/request.jl @@ -100,6 +100,9 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers TOO_MANY_REQUESTS = 429 EXPIRED_ERROR_CODES = ["ExpiredToken", "ExpiredTokenException", "RequestExpired"] REDIRECT_ERROR_CODES = [301, 302, 303, 304, 305, 307, 308] + + # https://boto3.amazonaws.com/v1/documentation/api/latest/guide/retries.html?highlight=retry + # https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-retries.html#cli-usage-retries-modes-standard.title THROTTLING_ERROR_CODES = [ "Throttling", "ThrottlingException", @@ -107,9 +110,13 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers "RequestThrottledException", "TooManyRequestsException", "ProvisionedThroughputExceededException", + "TransactionInProgressException", + "RequestLimitExceeded", + "BandwidthLimitExceeded", "LimitExceededException", "RequestThrottled", - "PriorRequestNotComplete", + "SlowDown", + "EC2ThrottledException", ] request.headers["User-Agent"] = user_agent[] @@ -119,11 +126,15 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers local aws_response local response + transient_retry = false + get_response = () -> begin credentials(aws) === nothing || sign!(aws, request) - aws_response = @mock _http_request(request.backend, request, stream) + aws_response = @mock _http_request( + request.backend, request, stream; transient_retry=transient_retry + ) response = aws_response.response if response.status in REDIRECT_ERROR_CODES @@ -154,6 +165,10 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers if !(e isa AWSException) return false end + if is_transient_error(e) + transient_retry = true + return true + end occursin("Signature expired", e.message) && return true @@ -202,7 +217,9 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers end end -function _http_request(http_backend::HTTPBackend, request::Request, response_stream::IO) +function _http_request( + http_backend::HTTPBackend, request::Request, response_stream::IO; transient_retry=false +) http_options = merge(http_backend.http_options, request.http_options) # HTTP options such as `status_exception` need to be used when creating the stack diff --git a/src/utilities/utilities.jl b/src/utilities/utilities.jl index cdc7ab2b2d..f90ee2e936 100644 --- a/src/utilities/utilities.jl +++ b/src/utilities/utilities.jl @@ -162,3 +162,20 @@ function Base.iterate(exp::AWSExponentialBackoff, i=1) delay = min(b * r^i, exp.max_backoff) return delay, i + 1 end + +# https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html +is_transient_error(e::HTTP.StatusError) = AWS._http_status(e) in (400, 408, 500, 502, 503, 504) + +# https://boto3.amazonaws.com/v1/documentation/api/latest/guide/retries.html?highlight=retry +# https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-retries.html#cli-usage-retries-modes-standard.title +const TRANSIENT_ERROR_CODES = ( + "RequestTimeout", + "RequestTimeoutException", + "PriorRequestNotComplete", + "ConnectionError", + "HTTPClientError" +) + +is_transient_error(e::AWSException) = e.code in TRANSIENT_ERROR_CODES || is_transient_error(e.cause) + +is_transient_error(::Downloads.RequestError) = true From d76619a19f710c56105fab7a605a8c2272784bfe Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 15:45:14 +0200 Subject: [PATCH 02/14] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/utilities/downloads_backend.jl | 4 +++- src/utilities/utilities.jl | 10 +++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 219e288918..85a95bfdc0 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -59,7 +59,9 @@ function read_body(x::IO) return read(x) end -function _http_request(backend::DownloadsBackend, request::Request, response_stream::IO; transient_retry=false) +function _http_request( + backend::DownloadsBackend, request::Request, response_stream::IO; transient_retry=false +) # HTTP.jl sets this header automatically. request.headers["Content-Length"] = string(body_length(request.content)) diff --git a/src/utilities/utilities.jl b/src/utilities/utilities.jl index f90ee2e936..74a245ba56 100644 --- a/src/utilities/utilities.jl +++ b/src/utilities/utilities.jl @@ -164,7 +164,9 @@ function Base.iterate(exp::AWSExponentialBackoff, i=1) end # https://docs.aws.amazon.com/sdkref/latest/guide/feature-retry-behavior.html -is_transient_error(e::HTTP.StatusError) = AWS._http_status(e) in (400, 408, 500, 502, 503, 504) +function is_transient_error(e::HTTP.StatusError) + return AWS._http_status(e) in (400, 408, 500, 502, 503, 504) +end # https://boto3.amazonaws.com/v1/documentation/api/latest/guide/retries.html?highlight=retry # https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-retries.html#cli-usage-retries-modes-standard.title @@ -173,9 +175,11 @@ const TRANSIENT_ERROR_CODES = ( "RequestTimeoutException", "PriorRequestNotComplete", "ConnectionError", - "HTTPClientError" + "HTTPClientError", ) -is_transient_error(e::AWSException) = e.code in TRANSIENT_ERROR_CODES || is_transient_error(e.cause) +function is_transient_error(e::AWSException) + return e.code in TRANSIENT_ERROR_CODES || is_transient_error(e.cause) +end is_transient_error(::Downloads.RequestError) = true From 062791b7985b01081d2714efdf84f868cd4d0a72 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 16:59:37 +0200 Subject: [PATCH 03/14] fix --- src/utilities/request.jl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/utilities/request.jl b/src/utilities/request.jl index 1a7843bfdd..489202f78d 100644 --- a/src/utilities/request.jl +++ b/src/utilities/request.jl @@ -187,6 +187,12 @@ function submit_request(aws::AbstractAWSConfig, request::Request; return_headers return true end + if e.code == "PriorRequestNotComplete" + # Retry this transient error, because the + # HTTP backend currently doesn't have a check for it. + 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") From ad02cbaf50a173d61070190e8cdc69ecad85e494 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 17:14:16 +0200 Subject: [PATCH 04/14] load AWSExceptions earlier to have the type --- src/AWS.jl | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/AWS.jl b/src/AWS.jl index fe09727a9f..82ad42a109 100644 --- a/src/AWS.jl +++ b/src/AWS.jl @@ -23,13 +23,17 @@ export JSONService, RestJSONService, RestXMLService, QueryService, set_features const DEFAULT_REGION = "us-east-1" -include(joinpath("utilities", "utilities.jl")) - include("AWSExceptions.jl") + +using ..AWSExceptions +using ..AWSExceptions: AWSException + + include("AWSCredentials.jl") include("AWSConfig.jl") include("AWSMetadata.jl") +include(joinpath("utilities", "utilities.jl")) include(joinpath("utilities", "request.jl")) include(joinpath("utilities", "response.jl")) include(joinpath("utilities", "sign.jl")) @@ -37,9 +41,6 @@ include(joinpath("utilities", "downloads_backend.jl")) include("deprecated.jl") -using ..AWSExceptions -using ..AWSExceptions: AWSException - const user_agent = Ref("AWS.jl/1.0.0") const aws_config = Ref{AbstractAWSConfig}() From a0d7e44364834b173e86018d380e771246be7ee0 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 17:17:01 +0200 Subject: [PATCH 05/14] Update src/AWS.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/AWS.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/src/AWS.jl b/src/AWS.jl index 82ad42a109..c0a5f870de 100644 --- a/src/AWS.jl +++ b/src/AWS.jl @@ -28,7 +28,6 @@ include("AWSExceptions.jl") using ..AWSExceptions using ..AWSExceptions: AWSException - include("AWSCredentials.jl") include("AWSConfig.jl") include("AWSMetadata.jl") From f931f27c97bbe17c044263a3ee518eeef9d6c720 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 17:31:53 +0200 Subject: [PATCH 06/14] fix --- test/patch.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/patch.jl b/test/patch.jl index 27b586285c..69022a02cd 100644 --- a/test/patch.jl +++ b/test/patch.jl @@ -62,7 +62,7 @@ function _response(; end function _aws_http_request_patch(response::AWS.Response=_response()) - p = @patch AWS._http_request(::AWS.AbstractBackend, request::Request, ::IO) = response + p = @patch AWS._http_request(::AWS.AbstractBackend, request::Request, ::IO; transient_retry=false) = response return p end From 9569e69a745341fb71f94e2ebdac181eb8122c46 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 17:35:47 +0200 Subject: [PATCH 07/14] add test --- test/issues.jl | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/test/issues.jl b/test/issues.jl index a5180b5bb1..c88caccaf4 100644 --- a/test/issues.jl +++ b/test/issues.jl @@ -218,14 +218,33 @@ 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) - - seekstart(io) - retrieved = read(io) - @test length(retrieved) == n - 1 - @test retrieved == data[1:(n - 1)] + if AWS.DEFAULT_BACKEND[] isa DownloadsBackend + # We'll use a different `DownloadsBackend` with an extra test. + called_make_new_downloader = false + make_new_downloader = () -> begin + called_make_new_downloader = true + return AWS.get_downloader(; fresh=true) + end + AWS.DEFAULT_BACKEND[] = DownloadsBackend(nothing, make_new_downloader) + end + try + 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)] + end + finally + # Reset the downloader + if AWS.DEFAULT_BACKEND[] isa DownloadsBackend + DEFAULT_BACKEND[] = DownloadsBackend() + end + end + if AWS.DEFAULT_BACKEND[] isa DownloadsBackend + @test called_make_new_downloader end end end From f633e83f837335259ea3987dcc0fd32b08e767d3 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Fri, 13 May 2022 17:36:11 +0200 Subject: [PATCH 08/14] Update test/patch.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- test/patch.jl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/patch.jl b/test/patch.jl index 69022a02cd..557dd0f050 100644 --- a/test/patch.jl +++ b/test/patch.jl @@ -62,7 +62,11 @@ function _response(; end function _aws_http_request_patch(response::AWS.Response=_response()) - p = @patch AWS._http_request(::AWS.AbstractBackend, request::Request, ::IO; transient_retry=false) = response + p = @patch function AWS._http_request( + ::AWS.AbstractBackend, request::Request, ::IO; transient_retry=false + ) + return response + end return p end From efc02a73f5e296867396e850274f54a8fefb31d2 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 15:05:02 +0200 Subject: [PATCH 09/14] format --- src/utilities/downloads_backend.jl | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 74f3d384a1..2cb5044013 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -86,14 +86,14 @@ function _http_request( local response check = function (s, e) - if is_transient_error(e) - # We want a new one, ref https://github.com/JuliaCloud/AWS.jl/issues/552 - downloader = backend.create_new_downloader() - return true - end - return (isa(e, HTTP.StatusError) && AWS._http_status(e) >= 500) || - isa(e, Downloads.RequestError) + if is_transient_error(e) + # We want a new one, ref https://github.com/JuliaCloud/AWS.jl/issues/552 + downloader = backend.create_new_downloader() + return true end + return (isa(e, HTTP.StatusError) && AWS._http_status(e) >= 500) || + isa(e, Downloads.RequestError) + end delays = AWSExponentialBackoff(; max_attempts=4) From d475d4d1a052d966579147684619ae94a01322c1 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 15:08:14 +0200 Subject: [PATCH 10/14] try matching existing retry behavior more closely --- src/utilities/downloads_backend.jl | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 2cb5044013..906b3dc4be 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -87,6 +87,13 @@ function _http_request( check = function (s, e) if is_transient_error(e) + + # TODO: remove this restriction. These valid transient errors + # are currently not supported by our tests. + if (isa(e, HTTP.StatusError) && AWS._http_status(e) < 500) + return false + end + # We want a new one, ref https://github.com/JuliaCloud/AWS.jl/issues/552 downloader = backend.create_new_downloader() return true From 19344a74d09126f80642c7e80fe0827851117e87 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 15:28:43 +0200 Subject: [PATCH 11/14] fix test --- test/issues.jl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/issues.jl b/test/issues.jl index c88caccaf4..9d1d609ea7 100644 --- a/test/issues.jl +++ b/test/issues.jl @@ -218,14 +218,14 @@ try end io = IOBuffer() - if AWS.DEFAULT_BACKEND[] isa DownloadsBackend + if AWS.DEFAULT_BACKEND[] isa AWS.DownloadsBackend # We'll use a different `DownloadsBackend` with an extra test. called_make_new_downloader = false make_new_downloader = () -> begin called_make_new_downloader = true return AWS.get_downloader(; fresh=true) end - AWS.DEFAULT_BACKEND[] = DownloadsBackend(nothing, make_new_downloader) + AWS.DEFAULT_BACKEND[] = AWS.DownloadsBackend(nothing, make_new_downloader) end try apply(_incomplete_patch(; data=data, num_attempts_to_fail=4)) do @@ -239,11 +239,11 @@ try end finally # Reset the downloader - if AWS.DEFAULT_BACKEND[] isa DownloadsBackend - DEFAULT_BACKEND[] = DownloadsBackend() + if AWS.DEFAULT_BACKEND[] isa AWS.DownloadsBackend + DEFAULT_BACKEND[] = AWS.DownloadsBackend() end end - if AWS.DEFAULT_BACKEND[] isa DownloadsBackend + if AWS.DEFAULT_BACKEND[] isa AWS.DownloadsBackend @test called_make_new_downloader end end From ae4f536f87975ecc62675170eaab0d30dc91a70a Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 17:05:19 +0200 Subject: [PATCH 12/14] update constructors + docs --- src/utilities/downloads_backend.jl | 40 +++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 1076c62fc7..0a7ed1a0b5 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -6,23 +6,38 @@ using HTTP.MessageRequest: body_was_streamed This backend uses the Downloads.jl stdlib to use libcurl as an HTTP client to connect to the AWS REST API. -It has two fields, - -- `downloader::Union{Nothing,Downloads.Downloader}`: if `nothing`, use a global Downloader object. Otherwise, uses the given Downloader. -- `create_new_downloader::Any`: a zero-argument function which returns a new Downloader object to use. - Defaults to creating a new global downloader. This is called when a transient error occurs. - Downloads.jl tends to perform better under concurrent operation than HTTP.jl, particularly with `@async` / `asyncmap`. As of March 2022, threading (e.g. `@spawn` or `@threads`) with Downloads.jl is broken on all releases of Julia ([Downloads.jl#110](https://github.com/JuliaLang/Downloads.jl/issues/110)), and there are still reported issues on the upcoming 1.7.3 and 1.8 releases ([Downloads.jl#182](https://github.com/JuliaLang/Downloads.jl/issues/182])). + +There are three constructors: + + DownloadsBackend() + +This constructor uses AWS.jl's global downloader object. If a transient error occurs, the global downloader object is replaced with a fresh one. + + DownloadsBackend(create_new_downloader) + +This constructor calls `create_new_downloader()` to create a new downloader which it uses. Upon encountering a transient error, another new downloader is created and used from then on. + + DownloadsBackend(D::Downloader) + +This constructor uses the provided downloader `D`, and uses it always. """ -struct DownloadsBackend <: AWS.AbstractBackend +mutable struct DownloadsBackend <: AWS.AbstractBackend downloader::Union{Nothing,Downloads.Downloader} + # `downloader_lock===nothing` signals that we don't want to replace + # the existing `downloader` field. + downloader_lock::Union{Nothing, ReentrantLock} create_new_downloader::Any end -DownloadsBackend() = DownloadsBackend(nothing, () -> get_downloader(; fresh=true)) -DownloadsBackend(D::Downloader) = DownloadsBackend(D, () -> get_downloader(; fresh=true)) +# Use global downloader; don't replace `downloader` field on transient errors +DownloadsBackend() = DownloadsBackend(nothing, nothing, () -> get_downloader(; fresh=true)) +# Use `create_new_downloader` to create a new `downloader`; DO replace the `downloader` field on transient errors +DownloadsBackend(create_new_downloader) = DownloadsBackend(create_new_downloader(), ReentrantLock(), create_new_downloader) +# Use provided downloader `D`; don't replace `downloader` on transient errors +DownloadsBackend(D::Downloader) = DownloadsBackend(D, nothing, () -> D) const AWS_DOWNLOADER = Ref{Union{Nothing,Downloader}}(nothing) const AWS_DOWNLOAD_LOCK = ReentrantLock() @@ -70,6 +85,13 @@ function _http_request( if transient_retry downloader = backend.create_new_downloader() + + # If we have a lock, use it to replace our existing downloader. + if backend.downloader_lock !== nothing + Base.@lock backend.downloader_lock begin + backend.downloader = downloader + end + end else downloader = @something(backend.downloader, get_downloader()) end From ba580696c3fd3af82a6fcf835beba4bab74c77c5 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 17:05:51 +0200 Subject: [PATCH 13/14] Update src/utilities/downloads_backend.jl Co-authored-by: Curtis Vogt --- src/utilities/downloads_backend.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 0a7ed1a0b5..6faa38adcd 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -51,7 +51,6 @@ function get_downloader(; fresh=false) downloader = nothing lock(AWS_DOWNLOAD_LOCK) do yield() # let other downloads finish - downloader isa Downloader && return nothing while true downloader = AWS_DOWNLOADER[] !fresh && downloader isa Downloader && return nothing From 169a9af6b71cca53bf9de5895ab9e52f2840e337 Mon Sep 17 00:00:00 2001 From: Eric Hanson <5846501+ericphanson@users.noreply.github.com> Date: Thu, 19 May 2022 17:09:45 +0200 Subject: [PATCH 14/14] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- src/utilities/downloads_backend.jl | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/utilities/downloads_backend.jl b/src/utilities/downloads_backend.jl index 6faa38adcd..3b36f8f509 100644 --- a/src/utilities/downloads_backend.jl +++ b/src/utilities/downloads_backend.jl @@ -28,14 +28,16 @@ mutable struct DownloadsBackend <: AWS.AbstractBackend downloader::Union{Nothing,Downloads.Downloader} # `downloader_lock===nothing` signals that we don't want to replace # the existing `downloader` field. - downloader_lock::Union{Nothing, ReentrantLock} + downloader_lock::Union{Nothing,ReentrantLock} create_new_downloader::Any end # Use global downloader; don't replace `downloader` field on transient errors DownloadsBackend() = DownloadsBackend(nothing, nothing, () -> get_downloader(; fresh=true)) # Use `create_new_downloader` to create a new `downloader`; DO replace the `downloader` field on transient errors -DownloadsBackend(create_new_downloader) = DownloadsBackend(create_new_downloader(), ReentrantLock(), create_new_downloader) +function DownloadsBackend(create_new_downloader) + return DownloadsBackend(create_new_downloader(), ReentrantLock(), create_new_downloader) +end # Use provided downloader `D`; don't replace `downloader` on transient errors DownloadsBackend(D::Downloader) = DownloadsBackend(D, nothing, () -> D)