bootstrap doesn't retry downloads that fail with a non-HTTP error

[jyn514]

We're passing --retry:

rust/src/bootstrap/download.rs

Line 220 in b80ee39

"--retry",

but by default that only retries 500 errors, not any error: https://everything.curl.dev/usingcurl/downloads/retry#retry-on-any-and-all-errors

Originally posted by @jyn514 in #109875 (comment)

[jyn514]

This is a regression from when I moved the downloads from python to rust (about a year ago now I think).

[albertlarsan68]

Mentoring instructions:

Add the --retry-all-errors option to the curl invocations, in

rust/src/bootstrap/bootstrap.py

Line 95 in b80ee39

"--retry", "3", "-Sf", url],

and

rust/src/bootstrap/download.rs

Line 220 in b80ee39

"--retry",

This doesn't affect the Windows Powershell fallback, since we already retry on any error.
There are many other curl invocations across the repo, but they seem to not be causing problems.

[kayagokalp]

@rustbot claim

[onur-ozkan]

This is quite new flag shipped with recent versions of curl, which is not supported on many machines/platforms(including our CI runners).

We can consider using --retry-connrefused as I also suggest at #110245 (comment)

[exdx]

I'd like to take a look at this. Looking through the PR history, we would like basic retry logic written in Rust versus using a curl flag like --retry to retry during the bootstrap process. If it's a 404, then do not retry, but any other errors should be retried. I assume there is a http library already vendored in, so no new dependencies should be added.

[exdx]

@rustbot claim

[onur-ozkan]

I'd like to take a look at this.

Thanks!

we would like basic retry logic written in Rust
I assume there is a http library already vendored in, so no new dependencies should be added.

This shouldn't be needed. For now, retrying on "connection refused" (#110178 (comment)) should be fine.

[exdx]

I'd like to take a look at this.

Thanks!

we would like basic retry logic written in Rust
I assume there is a http library already vendored in, so no new dependencies should be added.

This shouldn't be needed. For now, retrying on "connection refused" (#110178 (comment)) should be fine.

I'm a little confused as to why #110245 was closed then. What more is there beyond adding the retry refused connections flag?

[onur-ozkan]

I forgot that our CI runners depend on very old curl versions which doesn't support the flag I suggested. Sorry, you are right. As jyn also said(#110245 (comment)) in that PR, we can handle this in Rust

[exdx]

@onur-ozkan @jyn514 are we ok with adding the http dependency to facilitate sending requests to the payload URL? I'm very hesitant to add another crate dependency to the core payload. But without it, we would need to manually built a request via the standard library tcp library. Which is a little awkward.

http doesn't actually have support for sending requests. I suppose, in the case we decide to use it, another library would need to be added as well?

[onur-ozkan]

What about using curl arguments to read the status code and handle it in rust logic? It might be a bit ugly, but adding a dependency solely for this purpose seems like overkill to me.

[exdx]

What about using curl arguments to read the status code and handle it in rust logic? It might be a bit ugly, but adding a dependency solely for this purpose seems like overkill to me.

I agree adding a new dep seems extreme. I can try parsing the curl exit code, great idea.

[Vrajs16]

@rustbot claim

[danik292]

@rustbot claim

[danik292]

@jyn514 Hello Is this still relevant?

[jyn514]

please don't ping me about rust-lang/ issues. @onur-ozkan is the current maintainer.

[onur-ozkan]

@jyn514 Hello Is this still relevant?

We didn't change any curl flags since then; it should be still relevant.

[danik292]

@rustbot release-assignment