New Downloader for transient retries

[ericphanson]

fixes #552 for the Downloads backend.

Based on #548

It's a little awkward, because we have retries at 2 levels- on the level of the HTTP request (via whichever backend), and in submit_request. We don't have AWSExceptions until the 2nd layer, where we actually read from the stream. In the first layer, we don't read from the stream yet. So we need a way to tell _http_request that we are retrying from a transient error, so I added a new keyword argument.

Would really appreciate any thoughts.

[ericphanson]

This is a transient error, not a throttling one, according to https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-retries.html#cli-usage-retries-modes-standard.title

However, in this PR only Downloads backend is getting special transient error handling, so maybe I should add this back with a comment, so we don't mess up the HTTP behavior.

[ericphanson]

I added it later on as a separate check

[ericphanson]

bors try

[bors]

try

Build failed:

• Julia 1 - ubuntu-latest - x64

[ericphanson]

bors try

[bors]

try

Build failed:

• Julia 1.5 - ubuntu-latest - x64

[ericphanson]

bors try

[bors]

try

Build failed:

• Julia 1.5 - ubuntu-latest - x64

[ericphanson]

bors try

[bors]

try

Build failed:

• Julia 1.3 - ubuntu-latest - x64

[omus]

Suggested change

	create_new_downloader::Any
	create_new_downloader::Base.Callable

[ericphanson]

That isn't documented; is it really better than Any?

[omus]

Not to derail this PR but using undef when the downloader is not yet defined seems preferable

Suggested change

	const AWS_DOWNLOADER = Ref{Union{Nothing,Downloader}}(nothing)
	const AWS_DOWNLOADER = Ref{Downloader}()

You'd just need to change some code in get_downloader to use isassigned(AWS_DOWNLOADER) before dereferencing

[ericphanson]

Let's do that separately if we are going to change it. I vaguely remember some problems with that approach when trying it in the original implementation. Maybe it was useful to be able to reset back to nothing, or something like that...

[omus]

Why not restrict the constructor to only accepting create_new_downloader and the downloader field can just be used internally?

[ericphanson]

Downloaders are stateful, and the original API promise was that you could decide how to share them, e.g. you could have 1 downloader per thread that you provision out. However this issue shows that we need the ability to create new ones as well. That makes me think actually the original API was bad and we probably should only have as input create_new_downloader like you say. However I think that's probably breaking, since we started by allowing you to pass in a Downloader. So my compromise here is that we'll use whatever downloader you pass in, but sometimes we will make a new one.

But... it's even more complicated, because if we make a new one because we think the old one might have a problem, we don't want to use the old one anymore. But DownloadsBackend is immutable, so I don't have a way to replace the old one. We can replace AWS's global downloader, which is probably what most people are using, but if you were using the ability to provision downloaders on a per-request basis then we don't have a way to replace those.

This API problem still has me stumped. The current implementation ONLY fixes things in a good way for users of the global downloader.

[omus]

I would suggest you have the following for backwards compatibility:

DownloadsBackend(D::Downloader) = DownloadsBackend(() -> D)

Using this doesn't work with your transient fixes but is effectively just uses the old behaviour. If you want the fix you need to pass in a function. I can't see another option for this as we can't copy instances of Downloaders.

But... it's even more complicated, because if we make a new one because we think the old one might have a problem, we don't want to use the old one anymore. But DownloadsBackend is immutable

Why not make DownloadsBackend mutable or use a Ref for the downloader field?

[ericphanson]

I am worried about concurrent access to the field: we can have multiple readers and at least one writer to the field. I suppose we can add a lock though.

[ericphanson]

I've added a lock and updated the constructors