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

Assume http2 connection by default, instead of http1 #758

Merged
merged 10 commits into from
Aug 15, 2024

Conversation

ayush1794
Copy link
Contributor

@ayush1794 ayush1794 commented Aug 7, 2024

Since most of the servers now conform to http2, the change here updates the behaviour of assuming the connection to be http2 and not http1 by default. It will migrate to http1 if the server only supports http1.
One can set the httpVersion in ClientConfiguration to .http1Only which will start with http1 instead of http2.

Additional Changes:

  • Fixed an off by one error in the maximum additional general purpose connection check
  • Updated tests

@czechboy0
Copy link

@swift-server-bot test this please

@dnadoba
Copy link
Collaborator

dnadoba commented Aug 7, 2024

@swift-server-bot add to allowlist

@dnadoba dnadoba requested a review from fabianfett August 7, 2024 12:40
@dnadoba
Copy link
Collaborator

dnadoba commented Aug 7, 2024

LGTM. I'm wondering if we need to expose this settings not only through .http1Only but rather as initial preference that starts with http1 but still allows http2. @weissi what do you think?

@weissi
Copy link
Contributor

weissi commented Aug 7, 2024

LGTM. I'm wondering if we need to expose this settings not only through .http1Only but rather as initial preference that starts with http1 but still allows http2. @weissi what do you think?

TIL that we're not already doing that. Seems sensible to default to HTTP/2.

Regarding the 'prefer H1' override in public API: Yes, we'll likely need that but that could also be a follow-up?

@dnadoba
Copy link
Collaborator

dnadoba commented Aug 7, 2024

Yes, we'll likely need that but that could also be a follow-up?

agree

@ayush1794
Copy link
Contributor Author

ayush1794 commented Aug 7, 2024

Regarding the 'prefer H1' override in public API: Yes, we'll likely need that but that could also be a follow-up?

Just out of curiosity what would be a real use case for this?
If we do this, we should likely also need to change the ALPN ordering in that case to prefer h1 over h2 when TLS is needed (though again not sure of a TLS use case on h1)

@weissi
Copy link
Contributor

weissi commented Aug 7, 2024

Regarding the 'prefer H1' override in public API: Yes, we'll likely need that but that could also be a follow-up?

Just out of curiosity what would be a real use case for this? If we do this, we should likely also need to change the ALPN ordering in that case to prefer h1 over h2 when TLS is needed (though again not sure of a TLS use case on h1)

Four reasons come to mind:

  1. At the moment you can instruct AHC to create an arbitrary amount of HTTP/1.1 connections to a given host. That means you can for example have 1,000 concurrent requests to a given server. But at present, with H2, AHC will only ever do one connection which usually allows up to 100 concurrent requests (technically negotiated using MAX_CONCURRENT_STREAMS but in practice it's always 100)
  2. Buggy software where HTTP/1.1 works better than HTTP/2 but the software claims to support HTTP/2
  3. (very) Low-latency connections: HTTP/1.1 can be performant than HTTP/2 if you have essentially 0 latency (e.g. on localhost). HTTP/2 trades off more CPU usage on client & server for fewer roundtrips. That's usually a good tradeoff but the lower the latency, the worse it gets.
  4. Integration tests for server software that wants to check that HTTP/1.x also works. Of course this means we shouldn't actually have a preferHTTP1: Bool but rather {prefer,require}HTTPProtocolVersion: .v1_1 | .v2 | .v1_0

though again not sure of a TLS use case on h1

Today, the vast vast majority of (non-localhost) HTTP/1.x connections are via TLS. I'm not even sure if HTTP/2 is actually more prevalent on the internet than HTTP/1.1, haven't checked. Addition: Actually H2 and H3 should actually be more prevalent given that most of the internet sits behind the same handful of CDNs/Edges which will support H2, even if the upstream servers may only do H1.

@ayush1794
Copy link
Contributor Author

@swift-server-bot test this please

maximumConnectionUses: nil
)

var connectionIDs: [HTTPConnectionPool.Connection.ID] = []
for el in [el1, el2, el2] {
for el in [el1, el2] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you remove el2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we now directly start with h2 rather than h1 and then migrating to h1.
h2 has checks to not allow creating connections for same EL. The second el2 just fails the test because the check fails.

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this through!

@fabianfett fabianfett merged commit 1290119 into swift-server:main Aug 15, 2024
6 of 7 checks passed
@fabianfett fabianfett added the semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Aug 15, 2024
@ayush1794 ayush1794 deleted the http2 branch August 15, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants