HTTP/3: Change alt-svc protocol to h3 and add upgrade test #34469
Conversation
JamesNK
requested review from
BrennanConroy,
halter73 and
Tratcher
as code owners
ghost
added
the
| responseHeaders.HeaderAltSvc = $"h3-29=\":{option.IPEndPoint!.Port}\"; ma=84600"; | ||
| // TODO: Perf. Create string once instead of per-request. | ||
| // https://github.com/dotnet/aspnetcore/issues/34468 | ||
| responseHeaders.HeaderAltSvc = $"h3=\":{option.IPEndPoint!.Port}\"; ma=84600"; |
There was a problem hiding this comment.
Latest draft has finalized "h3"
There was a problem hiding this comment.
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.
I found the opposite with HttpClient: h3-29 doesn’t work and h3 does. That’s why I changed it in this PR.
There was a problem hiding this comment.
Chrome (and Edge) support "h3"
If HttpClient and major browsers support h3 then I think it's fine to completely switch in RC1.
| public async Task Listen_Http3AndSocketsCoexistOnSameEndpoint_AltSvcEnabled_Upgrade() | ||
| { | ||
| // Arrange | ||
| var builder = GetHostBuilder() |
There was a problem hiding this comment.
Doesn't the ALPN setting need to be updated to match?
There was a problem hiding this comment.
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.
Ok. The alt-svc and ALPN should be kept in sync, let's cover this in the meeting later today.
amcasey
added
area-networking
Fixes #34148