Add support for WebSockets over HTTP/2 #2894
Conversation
There was a problem hiding this comment.
This looks like a very high-quality PR, but I really don't know enough about websockets or HTTP/2 to review it.
@mladedav are you able to review this?
Also @SabrinaJewson would you be willing to maintain the websocket stuff going forward (be pinged on issues and PRs to help)?
|
I’m happy to be pinged! |
axum/src/extract/ws.rs
Outdated
| /// For HTTP/1.1 requests, this extractor requires the request method to be `GET`; | ||
| /// in later versions, `CONNECT` is used instead. Thus it should either be used | ||
| /// with [`any`](crate::routing::any), or placed behind | ||
| /// [`on`](crate::routing::on)`(`[`MethodFilter`]`::GET.or(`[`MethodFilter`]`::POST), ...)`. |
There was a problem hiding this comment.
I guess we're using POST here because we don't support CONNECT directly?
Do you think that we should add CONNECT as well?
There was a problem hiding this comment.
No, that’s just a typo on my part…
There was a problem hiding this comment.
Although I do now reälize that MethodFilter::CONNECT doesn’t exist. I pushed a commit to consistently use any instead.
| return RouteFuture::from_future(route.clone().oneshot_inner($req)) | ||
| .strip_body($method == Method::HEAD); | ||
| return route.clone().oneshot_inner($req); |
There was a problem hiding this comment.
Both PRs benefit from this change, so it can be merged in either order!
|
Done and done. |
|
Let me know if you want this backported (though maybe a follow-up PR adding |
Motivation
WebSockets over HTTP/2 are currently not supported. It is defined in RFC 8441 (websockets over HTTP/3 are defined in RFC 9220 and it is exactly the same as HTTP/2).
Solution
Implement the changes necessary to support them, which just requires looking at the
:protocolpseudo-header in the websocket extractor and advertising our support by enablingSETTINGS_ENABLE_CONNECT_PROTOCOLin HTTP/2.Additionally, I needed to fix the handling of responses to CONNECT requests: Axum was incorrectly adding the
Content-Lengthheader to them (see also hyperium/hyper#3748), which was causing things to break.