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

Fix HTTP/3 ALPN #56215

Merged
merged 3 commits into from
Jul 29, 2021
Merged

Fix HTTP/3 ALPN #56215

merged 3 commits into from
Jul 29, 2021

Conversation

CarnaViire
Copy link
Member

@CarnaViire CarnaViire commented Jul 23, 2021

Fixes #55894


Re #54726: I've taken a quick look on whether interop tests would start working after this. For Public_Interop_Upgrade_Success, it now works with LiteSpeed and Cloudflare servers and does not with Chromium one (error CONNECTION_IDLE). Public_Interop_Upgrade_Success still does not work at all, however the test itself seemed strange to me, but I didn't spend much time analyzing.

@ghost
Copy link

ghost commented Jul 23, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #55894

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@CarnaViire
Copy link
Member Author

/azp run runtime-libraries stress-http

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

Should we create test to verify that setting HTTP version to 3.0 will send correct/expected ALPN in handshake?

@CarnaViire
Copy link
Member Author

Should we create test to verify that setting HTTP version to 3.0 will send correct/expected ALPN in handshake?

I was observing transport (msquic) failures with ALPN_NEG_FAILURE when I've updated Http3Connection to "h3" but not yet updated Http3LoopbackServer, so existing tests kind-of check that implicitly. But I'll see how to create more explicit test.

@CarnaViire
Copy link
Member Author

I am observing a lot of stress failures for HTTP/3 (but it's not like all of them fail). I don't know whether it was this change that regressed it or some other change. I would expect it to fail completely if it was this change, so I have doubts, but I will investigate.

@wfurt
Copy link
Member

wfurt commented Jul 23, 2021

is there need to update asp.net as well?

@geoffkizer
Copy link
Contributor

Re this:

does not with Chromium one (error CONNECTION_IDLE).

This seems super weird, any idea what's up here?

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

👍

@CarnaViire
Copy link
Member Author

Stress regression is unrelated, shows on main, created an issue for that #56310

@CarnaViire
Copy link
Member Author

Should we create test to verify that setting HTTP version to 3.0 will send correct/expected ALPN in handshake?

@wfurt I wasn't able to come up with an idea on how to verify that explicitly. ALPN is hidden in QUIC handshake, isn't it? So I don't know how to intercept msquic to get that information... Do you have some ideas on how I could achieve that?


does not with Chromium one (error CONNECTION_IDLE).

This seems super weird, any idea what's up here?

@geoffkizer It could be that the server just isn't working for some reason, but we need to investigate in #54726


is there need to update asp.net as well?

@wfurt ASP.NET Core tests will need to be updated
https://github.com/dotnet/aspnetcore/blob/9da42b9fab4c61fe46627ac0c6877905ec845d5a/src/Servers/Kestrel/Transport.Quic/test/QuicTestHelpers.cs#L28 (cc @JamesNK)
But QuicTransportOptions does not seem to have any default value https://github.com/dotnet/aspnetcore/blob/8b30d862de6c9146f466061d51aa3f1414ee2337/src/Servers/Kestrel/Transport.Quic/src/QuicTransportOptions.cs In stress tests I specified ALPN manually for Kestrel https://github.com/dotnet/runtime/pull/56215/files#diff-73d3348ab4b049ae3561a8faf2c2ea98d1a379e035ecb65ccf5f21801b41a07fR130 And as I don't see any ALPN_NEG_FAILURE there and success rate is same as in main, I guess that's ok

@wfurt
Copy link
Member

wfurt commented Jul 26, 2021

I don't think we need to hold back for the test @CarnaViire. It would be nice. I see two possible strategies. QuicConnection has NegotiatedApplicationProtocol. And the handshake happens early on. So it seems like if you create QuicListener and do HttpRequest to it you should know - even if everything else fails. Alternatively we could update Loopback server and/or get access to it via some reflection.

@JamesNK
Copy link
Member

JamesNK commented Jul 26, 2021

is there need to update asp.net as well?

We'll update our configurable ALPN to h3. The property will probably be removed for RC1.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt wfurt merged commit 121baa8 into dotnet:main Jul 29, 2021
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HTTP/3] Update ALPN to h3
5 participants