fix: disable multiplexing for some versions of curl

[weihanglo]

What does this PR try to resolve?

Fixes #12202.
Zulip topic.

In certain versions of libcurl when proxy is in use with HTTP/2
multiplexing, connections will continue stacking up. This was
fixed in libcurl 8.0.0 in curl/curl@821f6e2

However, Cargo can still link against old system libcurl if it is from a
custom built one or on macOS. For those cases, multiplexing needs to be
disabled when those versions are detected.

How should we test and review this PR?

The first commit refactors how Cargo gets http.proxy config, so that
wecan have the detection inside Config. Please call it out if you
feel uncomfortable to this.

The second one did the trick, and a unit test is added.

Manual test the behavior

To manually test the behavior, you need to build cargo with
the all-static feature, which despite its name links libcurl
dynamically on macOS. It is a feature used by rustup distributions.

cargo b -F all-static

I don't really understand curl internals so had no luck to figure
out the relationship between proxies and HTTP/2 multiplexing.
I use hinata/nginx-forward-proxy to spin up a proxy to test curl's behavior.
It turns out that if there are fewer dependencies to download,
or we set MAX_CONCURRENT_STREAMS to a lower value in curl,
it largely affects the happening of the error no filter connected.

That implies multiplexing and http proxy are really influencing
each other in some way.

My patch to curl and curl-sys

diff --git a/Cargo.toml b/Cargo.toml
index ec9f878b3..3a687c309 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -205,3 +205,7 @@ vendored-libgit2 = ["libgit2-sys/vendored"]
 pretty-env-logger = ["pretty_env_logger"]
 # This is primarily used by rust-lang/rust distributing cargo the executable.
 all-static = ['vendored-openssl', 'curl/static-curl', 'curl/force-system-lib-on-osx']
+
+[patch.crates-io]
+curl = { git = "https://github.com/weihanglo/curl-rust", branch = "curlmopt" }
+curl-sys = { git = "https://github.com/weihanglo/curl-rust", branch = "curlmopt" }
diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs
index 7e1f2a587..21ae3ca10 100644
--- a/src/cargo/sources/registry/http_remote.rs
+++ b/src/cargo/sources/registry/http_remote.rs
@@ -246,6 +246,8 @@ impl<'cfg> HttpRegistry<'cfg> {
                 .status("Updating", self.source_id.display_index())?;
         }
 
+        self.multi.set_max_concurrent_streams(10)?;
+
         Ok(())
     }

[rustbot]

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

[weihanglo]

BTW. Although this is not really a regression, with sparse registry the number of connections is easier to exceed MAX_CONCURRENT_STREAMS (default to 100 in libcurl). Should we backport it at least for beta-nominated Nominated to backport to the beta branch. ?

[ehuss]

I'm a little uncertain about making this macos only. What would be the concern with just checking this on all platforms?

[weihanglo]

That was just me trying to shave off all old system libcurl aggressively 😅.

The constraint has been removed.

And weird. I forced pushed something but it didn't show up in this pull request.

[ehuss]

There's an ongoing incident: https://www.githubstatus.com/incidents/1g1gkh0qpyvs

[ehuss]

If you want to backport this to beta, that sounds good to me!

I think at some point it would be good to set up a proxy test using the container tests.

[weihanglo]

Thanks for the review!

@bors r=ehuss

I think at some point it would be good to set up a proxy test using the container tests.

Was thinking the same thing, though it feels a bit too hard to write one. I'll try when I have time. Thanks for the suggestion!

[bors]

📌 Commit 6af8cb2 has been approved by ehuss

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6af8cb2 with merge 41d662b...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 41d662b to master...