-
Notifications
You must be signed in to change notification settings - Fork 10k
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
HTTP/3: Change alt-svc protocol to h3 and add upgrade test #34469
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -152,6 +152,73 @@ public async Task Listen_Http3AndSocketsCoexistOnSameEndpoint_ClientSuccess() | |||
await host.StopAsync().DefaultTimeout(); | ||||
} | ||||
|
||||
[ConditionalFact] | ||||
[MsQuicSupported] | ||||
public async Task Listen_Http3AndSocketsCoexistOnSameEndpoint_AltSvcEnabled_Upgrade() | ||||
{ | ||||
// Arrange | ||||
var builder = GetHostBuilder() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't the ALPN setting need to be updated to match?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HttpClient is being inconsistent and using h3 in the altsvc but not as it’s alpn dotnet/runtime#55894 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. The alt-svc and ALPN should be kept in sync, let's cover this in the meeting later today. |
||||
.ConfigureWebHost(webHostBuilder => | ||||
{ | ||||
webHostBuilder | ||||
.UseKestrel(o => | ||||
{ | ||||
o.EnableAltSvc = true; | ||||
o.Listen(IPAddress.Parse("127.0.0.1"), 0, listenOptions => | ||||
{ | ||||
listenOptions.Protocols = Core.HttpProtocols.Http1AndHttp2AndHttp3; | ||||
listenOptions.UseHttps(); | ||||
}); | ||||
}) | ||||
.Configure(app => | ||||
{ | ||||
app.Run(async context => | ||||
{ | ||||
await context.Response.WriteAsync("hello, world"); | ||||
}); | ||||
}); | ||||
}) | ||||
.ConfigureServices(AddTestLogging); | ||||
|
||||
using var host = builder.Build(); | ||||
await host.StartAsync().DefaultTimeout(); | ||||
|
||||
var httpClientHandler = new HttpClientHandler(); | ||||
httpClientHandler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator; | ||||
|
||||
using (var client = new HttpClient(httpClientHandler)) | ||||
{ | ||||
// Act | ||||
var request1 = new HttpRequestMessage(HttpMethod.Get, $"https://127.0.0.1:{host.GetPort()}/"); | ||||
request1.VersionPolicy = HttpVersionPolicy.RequestVersionOrHigher; | ||||
var response1 = await client.SendAsync(request1).DefaultTimeout(); | ||||
|
||||
// Assert | ||||
response1.EnsureSuccessStatusCode(); | ||||
Assert.Equal(HttpVersion.Version20, response1.Version); | ||||
var responseText1 = await response1.Content.ReadAsStringAsync().DefaultTimeout(); | ||||
Assert.Equal("hello, world", responseText1); | ||||
|
||||
Assert.True(response1.Headers.TryGetValues("alt-svc", out var altSvcValues)); | ||||
Assert.Single(altSvcValues, @$"h3="":{host.GetPort()}""; ma=84600"); | ||||
|
||||
// Act | ||||
var request2 = new HttpRequestMessage(HttpMethod.Get, $"https://127.0.0.1:{host.GetPort()}/"); | ||||
request2.VersionPolicy = HttpVersionPolicy.RequestVersionOrHigher; | ||||
var response2 = await client.SendAsync(request2).DefaultTimeout(); | ||||
|
||||
// Assert | ||||
response2.EnsureSuccessStatusCode(); | ||||
Assert.Equal(HttpVersion.Version30, response2.Version); | ||||
var responseText2 = await response2.Content.ReadAsStringAsync().DefaultTimeout(); | ||||
Assert.Equal("hello, world", responseText2); | ||||
|
||||
Assert.False(response2.Headers.Contains("alt-svc")); | ||||
} | ||||
|
||||
await host.StopAsync().DefaultTimeout(); | ||||
} | ||||
|
||||
private static async Task CallHttp3AndHttp1EndpointsAsync(int http3Port, int http1Port) | ||||
{ | ||||
// HTTP/3 | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest draft has finalized "h3"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was testing the ALPN code with similar values. I think we should include both h3 and h3-29 for compat for at least a milestone, that's really common on websites I've checked. h3 alpn didn't work with HttpClient yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the opposite with HttpClient: h3-29 doesn’t work and h3 does. That’s why I changed it in this PR.
https://github.com/dotnet/runtime/blob/d3a8a19db1d8df774927f35f3f6d393ffbd075cb/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L1118
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome (and Edge) support "h3"
If HttpClient and major browsers support h3 then I think it's fine to completely switch in RC1.