Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable HTTP/2 by default #6271

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Enable HTTP/2 by default #6271

merged 1 commit into from
Nov 9, 2018

Conversation

alexcrichton
Copy link
Member

This commit switches Cargo to using HTTP/2 by default. This is
controlled via the http.multiplexing configuration variable and has
been messaged out for testing 1 (although got very few responses).

There's been surprisingly little fallout from parallel downloads, so
let's see how this goes!

@alexcrichton
Copy link
Member Author

r? @dwijnand

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2018

I've been meaning to point out, I think it has been using HTTP/2 on some platforms already. The default in libcurl if you don't set http_version is to let the backend decide. On openssl, it looks like it uses http/2 if available (darwinssl does not, not sure what windows does). It was just using HTTP/2 without multiplexing.

@alexcrichton
Copy link
Member Author

Oh dear it looks like you're right! It appears setting multiplexing = true doesn't even use HTTP/2 on OSX, and on Linux it is indeed already being used. (away from Windows right now to test that)

I've pushed a commit to forcibly disable HTTP/2 if multiplexing is false, let me take a closer look at curl on osx

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2018

doesn't even use HTTP/2 on OSX

Oh, that's why I posted alexcrichton/curl-rust#237, it should work when multiplexing=true with that patch (or if you build non-static with the system libcurl which uses libressl backend).

@alexcrichton alexcrichton force-pushed the h2 branch 2 times, most recently from 2b7aef8 to 2a7bb9e Compare November 6, 2018 22:30
@alexcrichton
Copy link
Member Author

Ah yes, indeed!

I've hopefully fixed the Travis errors on OSX with this PR to auto-enable static-curl. Once that's green on CI I'll merge.

I think we'll have to back out alexcrichton/curl-rust#237 unfortunately though. While it works on newer systems it produces link errors when compiled with MACOSX_DEPLOYMENT_TARGET=10.8 in the environment (which is what Rust's build configures). I was testing that locally to see what symbols were required and whether it would work and ran into that.

In good news though I did indeed confirm that alexcrichton/curl-rust#237 allowed usage of HTTP/2.

One option we could do is to go back to not statically linking libcurl into the Cargo binary on OSX, gracefully handling errors at runtime. That way if we're running on a modern system (which we likely are) then the system libcurl will have all the right HTTP/2 features. If we're running on an older system (such as 10.7-compatible ones) they won't have the HTTP/2 features and we'll just get runtime errors.

Perhaps the best strategy to take here?

@alexcrichton
Copy link
Member Author

Oh one other strategy is to get curl to do the HTTP/2 upgrade without ALPN (which requires the newer functions and such) but instead with normal Upgrade: ... headers. I can't seem to figure out how to get curl to do that though?

@alexcrichton
Copy link
Member Author

I can't seem to figure out how to get curl to do that though?

Aha. Taking a look at the relevant code the Upgrade: ... path is only enabled if the connection is without SSL, so I don't think curl supports an HTTP/2 upgrade over SSL without ALPN

@sfackler
Copy link
Member

sfackler commented Nov 6, 2018

The HTTP/2 spec mandates ALPN for HTTP/2 over TLS.

@alexcrichton
Copy link
Member Author

Makes sense! The two options I think we have (specifically for OSX) are:

  • Statically link libcurl. We give up ever using HTTP/2 until we can increase the minimum binary version.
  • Dynamically link libcurl. Ignore errors trying to enable HTTP/2. On modern systems HTTP/2 should get enabled. On older systems HTTP/1.1 will be used (but more parallel) and still see a nice boost.

I'd lean towards the latter option, thoughts @ehuss? To do that we'd need to revert alexcrichton/curl-rust#237 and probably add a feature like no-really-force-the-system-libcurl for rustbuild to enable on OSX.

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2018

Oh, sorry, I didn't think to check the deployment version. I just saw xcode9.3-moar and thought that would be sufficient. I think that stuff was added in 10.12. 😢

Just to clarify some things:

  • If it is dynamically linked, you just have to be careful to only use symbols from the oldest version of libcurl (whatever is on 10.7?)?
  • Why is darwinssl used over openssl? Is it for certificates?

@alexcrichton
Copy link
Member Author

Heh no worries, we would have eventually caught it on CI anyway :)

It turns out libcurl makes the symbol problem very easy for us, they basically never add symbols. Almost everything new gets added as new options to existing apis, so all the older versions just return errors about unknown options. FWIW libcurl is one of the very few libraries I've seen have this kind of ABI compat, but it's really nice in situations like this :)

I've never actually tried using OpenSSL with curl on OSX. I've just sort of naively assumed that it uses a different certificate store or isn't compatible with the syste certificate store in one way or another. But yeah certificates are the main concern/reason for using the system libraries.

@ehuss
Copy link
Contributor

ehuss commented Nov 6, 2018

Hm, yea, it's probably safest to dynamically link, then. Since 10.13, system curl switched to libressl, but I imagine with some patches to use the macos keychain files. Probably don't want to deal with that. Let me know if you want me to do anything to help.

This commit switches Cargo to using HTTP/2 by default. This is
controlled via the `http.multiplexing` configuration variable and has
been messaged out for testing [1] (although got very few responses).

There's been surprisingly little fallout from parallel downloads, so
let's see how this goes!

[1]: https://internals.rust-lang.org/t/testing-cargos-parallel-downloads/8466
@alexcrichton
Copy link
Member Author

Ok I've updated this with the current state of play. When this lands it will fail any update of rust-lang/rust until we also update rust-lang/rust to force usage of the system libcurl, which I can take care of after this lands.

@dwijnand
Copy link
Member

dwijnand commented Nov 7, 2018

I think it makes sense on this one to...

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned dwijnand Nov 7, 2018
@ehuss
Copy link
Contributor

ehuss commented Nov 9, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2018

📌 Commit c181f49 has been approved by ehuss

@bors
Copy link
Contributor

bors commented Nov 9, 2018

⌛ Testing commit c181f49 with merge 241fac0...

bors added a commit that referenced this pull request Nov 9, 2018
Enable HTTP/2 by default

This commit switches Cargo to using HTTP/2 by default. This is
controlled via the `http.multiplexing` configuration variable and has
been messaged out for testing [1] (although got very few responses).

There's been surprisingly little fallout from parallel downloads, so
let's see how this goes!

[1]: https://internals.rust-lang.org/t/testing-cargos-parallel-downloads/8466
@bors
Copy link
Contributor

bors commented Nov 9, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: ehuss
Pushing 241fac0 to master...

@bors bors merged commit c181f49 into rust-lang:master Nov 9, 2018
@alexcrichton alexcrichton deleted the h2 branch November 14, 2018 15:24
@alexcrichton
Copy link
Member Author

@ehuss I remembered to actually test this today locally, and it looks like nightly Cargo is indeed linked against the system libcurl and, as expected, I see http/2 downloads happening by default! I don't have an older machine without http/2 support to test out, but I'm not too too worried about it.

@ehuss
Copy link
Contributor

ehuss commented Nov 17, 2018

Nice! I've been looking at getting some older versions of MacOS, but Apple makes it difficult (you can no longer download them from Mojave).

@ehuss ehuss added this to the 1.32.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants