-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Implement Http/2 WebSockets #41558
Implement Http/2 WebSockets #41558
Conversation
src/Middleware/WebSockets/src/Microsoft.AspNetCore.WebSockets.csproj
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.Generated.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2WebSocketTests.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.FeatureCollection.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2WebSocketTests.cs
Show resolved
Hide resolved
src/Middleware/WebSockets/samples/EchoApp/Properties/launchSettings.json
Show resolved
Hide resolved
httpContext.Features.Set<IHttpExtendedConnectFeature>(new ConnectFeature() | ||
{ | ||
IsExtendedConnect = true, | ||
Protocol = "WebSocket", |
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.
Can we test sending and receiving data frames over the WebSocket and then gracefully closing it here? I understand that nothing should change relative to HTTP/1.1 WebSockets once you get the underlying Stream, but it's good to verify.
It's also tempting to open up the stream and connection windows in both directions Http2ConnectionTests and create a duplex Stream over the data frames for the request and wrap that in a client WebSocket so we can get better end-to-end testing of this scenario since we don't have any HttpClient or JS WebSocket based tests for the end-to-end. That'd be a more work, but probably not too much.
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'm planning to add more functional tests once the HttpClient implementation is available. Without that it's pretty hard to write any realistic tests. The client team has asked us to merge this ASAP so they can use it develop the client.
@Tratcher, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
As of dotnet#41558, four settings are expected, not three. Bonus: Actually show the http2cat logging
* Fix http2cat sample As of #41558, four settings are expected, not three.
Fixes #7801
This adds support to Kestrel and the WebSocket middleware for HTTP/2 WebSockets. It's enabled by default in both layers and does not currently provide an opt-out. Browsers silently utilize HTTP/2 WebSockets when advertised by the server. HttpClient support is coming soon, when we have that we'll write some interop tests (#41895).