-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Properly respond with sec-websocket-protocol under http/2 #3141
base: main
Are you sure you want to change the base?
Properly respond with sec-websocket-protocol under http/2 #3141
Conversation
Ah, looks good! Can you modify the |
9a4f267
to
95f2535
Compare
* Port HTTP/2 support. * Port tokio-rs/axum#3141
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 could click on "Approve" but I have very little knowledge about WebSocket internals, so I'd rather have those with more experience in this area chime in 😅
Same here, no knowledge in WS in HTTP2. @Turbo87 You haven't approved AFAIS: |
yeah, that was intentional. I didn't want to approve anything I don't have a clue about :D |
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.
Yeah, sorry I didn’t say before but this looks good.
Motivation
Was looking at the
ws
code and noticed that the subprotocol never gets properly communicated to the client if the connection is HTTP/2.Solution
Add the
sec-websocket-protocol
header irrespective of HTTP version. A response with that header is still a valid HTTP/2 websocket handshake, as per RFC8441.