Fix WebSocket frame concatenation for Netty #3801
Conversation
| @@ -101,6 +101,8 @@ private[http4s] object Http4sWebSockets { | |||
| case (None, f: WebSocketFrame.Pong) => (None, Some(f)) | |||
| case (None, f: WebSocketFrame.Close) => (None, Some(f)) | |||
| case (None, f: WebSocketFrame.Data[_]) if f.finalFragment => (None, Some(f)) | |||
| case (None, f: WebSocketFrame.Text) => (Some(Right(f.payload)), None) | |||
There was a problem hiding this comment.
indeed this seems rather basic :D
There was a problem hiding this comment.
but if http4s concatenates frames by itself, shouldn't we remove this? it's dead code anyway?
There was a problem hiding this comment.
Yeah, let me double check though, maybe there's some kind of switch on the Blaze/Ember level to control this.
There was a problem hiding this comment.
Ok, turns out that:
- Http4s can be configured to not concatenate fragmented frames and not ignore pings/pongs on the level of
WebSocketBuilder2:
.withHttpWebSocketApp(wsb => Router("/" -> wsRoutes(wsb.withDefragment(false).withFilterPingPongs(false)).orNotFound)- This has no effect if used with Blaze, which always automatically defragments and filters pings pongs
- It works with Ember though.
I'll remove these stages from Http4sWebSockets and add some documentation.
There was a problem hiding this comment.
Or we can keep our own defragmentation and force wsb.withDefragment(false) to make this Tapir-native switch work on Ember by forcing passing through our concatenation 🤔
There was a problem hiding this comment.
Ah ... if it's optional, let's keep the code, and let the user of http4s decide. So I guess the code is good as-is
There was a problem hiding this comment.
It has several bugs actually, as I discovered when I switched from Blaze to Ember in tests. Fixes are on the way in a separate PR then ;)
When handling incoming WebSocket frames, there was a missing case for a "non-final data frame and no data accumulated".
WebSocketFrameConverters, so it's shared between netty-sync and netty-catsWebSocketContinuationFrameis a binary frame that follows a text frame).BTW it turns out http4s does frame concatenation by itself, regardless of our configuration.
After adding missing cases, there are issues with concatenation in zio-http and vert.x. The stream freezes after receiving first frame in the accumulation. This needs separate investigation.