[djc]

Fixes #5176.

(With help from Claude.)

[rami3l]

@iliana @Skgland Would you mind giving our test build a try and see if rustup can better handle your network connection? You can download the corresponding one for your host at https://github.com/rust-lang/rustup/actions/runs/41038394164 🙏

[rami3l]

Question: Is this payload always ignored, and if so maybe we should just forget about it?

View changes since the review

[djc]

Not sure I understand what you mean. We're streaming frames of data from the HTTP response onto disk. When an error occurs after we've streamed some frames, why not preserve what we've downloaded so we can resume from it later?

[rami3l]

Sorry for not being super clear previously, but I mean that payload in DownloadError::Interrupted(payload) is always matched as _ (discarded). Does this mean that there is no point in saving it in the first place?

[rami3l]

LGTM, although I'd love if this can be tested before being merged 🙏

PS: Also changed the patch's title to make it clear that this is resuming server-interrupted downloads (instead of user-interrupted downloads which we already support).

View changes since this review

[djc]

LGTM, although I'd love if this can be tested before being merged 🙏

Testing it isn't really trivial -- the dist test fixtures are using file:// URLs and so far we don't have a HTTP server for them AIUI. We'd also need a much larger file.

[rami3l]

LGTM, although I'd love if this can be tested before being merged 🙏

Testing it isn't really trivial -- the dist test fixtures are using file:// URLs and so far we don't have a HTTP server for them AIUI. We'd also need a much larger file.

I totally understand that, and that is why I think some manual testing could be helpful here. 🙏

[Skgland]

@iliana @Skgland Would you mind giving our test build a try and see if rustup can better handle your network connection? You can download the corresponding one for your host at https://github.com/rust-lang/rustup/actions/runs/41038394164 🙏

I won't be able to try it before sunday.

[iliana]

With this change on my system I am now able to get toolchains to install, although I did hit:

info: checking for self-update (current version: 1.30.0)
info: downloading self-update (new version: 1.29.2)
error: could not download file from 'https://static.rust-lang.org/rustup/archive/1.29.2/x86_64-unknown-linux-gnu/rustup-init' to '/home/iliana/.rustup/self-update/rustup-init': error decoding response body: request or response body error: error reading a body from connection: connection reset

The behavior is now tolerable but still annoying compared to the curl backend due to what seem like sleeps/exponential backoff between retries. curl might be experiencing the same connection resets but retrying without any backoff, I'm not entirely sure.

[rami3l]

With this change on my system I am now able to get toolchains to install, although I did hit:

info: checking for self-update (current version: 1.30.0)
info: downloading self-update (new version: 1.29.2)
error: could not download file from 'https://static.rust-lang.org/rustup/archive/1.29.2/x86_64-unknown-linux-gnu/rustup-init' to '/home/iliana/.rustup/self-update/rustup-init': error decoding response body: request or response body error: error reading a body from connection: connection reset

The behavior is now tolerable but still annoying compared to the curl backend due to what seem like sleeps/exponential backoff between retries. curl might be experiencing the same connection resets but retrying without any backoff, I'm not entirely sure.

@iliana As I explained above, rustup's curl backend doesn't expose this behavior because it's too old to support http2; we are trying to improve http2 support here so there is no parity to be reached WRT the libcurl backend. Removing http2 support or defaulting to http1.1 when using the reqwest backend is NOT an option here for us.

[iliana]

That makes sense! Sorry I missed that explanation.

[djc]

The behavior is now tolerable but still annoying compared to the curl backend due to what seem like sleeps/exponential backoff between retries. curl might be experiencing the same connection resets but retrying without any backoff, I'm not entirely sure.

How do you sleeps/exponential backoffs? I don't think we have those? If there is something we could maybe avoid it for this particular failure mode.

[djc]

@iliana As I explained above, rustup's curl backend doesn't expose this behavior because it's too old to support http2; we are trying to improve http2 support here so there is no parity to be reached WRT the libcurl backend. Removing http2 support or defaulting to http1.1 when using the reqwest backend is NOT an option here for us.

But, maybe there is a way to fall back to HTTP/1 in failure cases? Not sure how easy reqwest makes that sort of thing.

[iliana]

How do you sleeps/exponential backoffs? I don't think we have those?

Here's what I see: https://asciinema.org/a/7eDycB7rXUMn1WLX

This may simply be the amount of time it takes to start the connection + request again?

[djc]

How do you sleeps/exponential backoffs? I don't think we have those?

Here's what I see: https://asciinema.org/a/7eDycB7rXUMn1WLX

This may simply be the amount of time it takes to start the connection + request again?

Yeah, the delay in retrying is a little surprising. Also, I guess we should retry the rustup self-update download, as well?

Still, seems like this PR provides a decent improvement, in that your toolchain component downloads can now actually complete.