ref(download): Retry downloads to S3/GCS/HTTP/local FS sources

[relaxolotl]

Rudimentary retry logic has been added to requests to download files from all supported sources. The existing retry implementation for Sentry-hosted sources was used as a template for these changes.

This branches off of the changes made in #483, and applies changes made in #395 to all sources. It may be useful to consider whether we want to deploy these together with circuit breaking as implemented in #397, but I'm not sure if I'm the right person to make that call.

Some concerns about the implementation:

• If the stream encounters any issues and exits early, the current retry logic may be flawed in that it re-attempts both the initial request as well as the stream from the beginning. This would be particularly painful if a stream errors out just before it is complete.
• The retry counter is shared between both the initial request to the source as well as the stream. It may not be a good idea to do so, and in the future it might be preferable to have different retry counters for those two requests.

I'm unsure if these two issues are blockers for the change though. I don't believe this change necessarily makes it more difficult to fix either of those making them non-blockers, but I'm interested in what others might think about these two problems.

[Swatinem]

I have quite little insight into this, but no objections

[flub]

AFAIK this now does exactly the same for all the sources. Would it make sense to move the retry logic out of the source-specific code and write it once in mod.rs?

[flub]

Are filesystem failures really worth retrying? The chances of them improving are very slim I'd say. Though it also doesn't do any harm I guess.

[relaxolotl]

Good point: I figured it'd be better to just make sure these are all consistent instead of making more exceptions for filesystem downloads. It does add some amount of complexity to their implementation, but I think the tradeoff is fine for now.

[flub]

• If the stream encounters any issues and exits early, the current retry logic may be flawed in that it re-attempts both the initial request as well as the stream from the beginning. This would be particularly painful if a stream errors out just before it is complete.

I think this is fine, AFAIK there's nothing that can be done about that though? If the stream fails you always need to make the GET request again from a network point of view i think?

• The retry counter is shared between both the initial request to the source as well as the stream. It may not be a good idea to do so, and in the future it might be preferable to have different retry counters for those two requests.

I think for retrying its fine to not worry why we are retrying. (though let's see how long before i have to retract that statement 😉 )

[flub]

AFAIK this now does exactly the same for all the sources. Would it make sense to move the retry logic out of the source-specific code and write it once in mod.rs?

If you disagree with this and would prefer to merge as-is, I'm not going to object either btw 😃

[Swatinem]

I will un-approve, lets rebase this and maybe unify the retry logic and move it into DownloadService::download even?

[relaxolotl]

I'm going to rebase this off of #489 since both of these PRs touch the same places in the code.

[relaxolotl]

Rebased off of #489 and unified the retry logic, as suggested by @flub and @Swatinem 🎉 A final review from either of you would be great, especially since the diff has changed significantly in structure.

I'm going to pull in @loewenheim just to make sure I haven't mucked anything up for him: Could I bug you to take a quick look over the changes here?

[Swatinem]

Move the retry call up and around the match. I see no reason why it should be duplicated in each match arm.

[relaxolotl]

The gotcha to this is that I'm unsure how to coerce the types so that I can wrap the match in the retry. Moving it up a level produces the following error:

`match` arms have incompatible types
while checking the return type of the `async fn`
    expected type `impl futures::Future` (opaque type at <crates/symbolicator/src/services/download/sentry.rs:245:10>)
found opaque type `impl futures::Future` (opaque type at <crates/symbolicator/src/services/download/http.rs:71:10>)
distinct uses of `impl Trait` result in different opaque types

The retry expects a closure that returns a Future, but the futures returned by every specific download_source implementation are all different. In other words, if we were to move the retry up to wrap the match, every branch of that match is returning a different Future implementation.

I could be missing a trick here to make this work. There's future::Either that might work, but it only combines two Futures and I'd have to chain several of them for this particular use case. Thoughts?

Something to note is that the original implementation also awaits on the future inside of every match branch to narrow the type down to just Result<DownloadStatus, DownloadError>, presumably to side-step the same issue.