Add support for per message deflate to websocket connectinos#2536
Closed
dbinnersley wants to merge 1 commit intowundergraph:mainfrom
Closed
Add support for per message deflate to websocket connectinos#2536dbinnersley wants to merge 1 commit intowundergraph:mainfrom
dbinnersley wants to merge 1 commit intowundergraph:mainfrom
Conversation
dbinnersley
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Contributor
WalkthroughThis PR adds per-message deflate compression support for GraphQL WebSocket connections. Changes include: configuration schema and types for compression settings, compression negotiation and frame handling logic in the WebSocket core, test infrastructure with compression-enabled dialers, and comprehensive test scenarios validating compression behavior across different configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/websocket.go (1)
162-302:⚠️ Potential issue | 🟠 MajorHandle fragmented frames when compression is enabled.
The current implementation will fail to correctly process fragmented compressed messages. According to gobwas/ws RFC 7692 implementation,
wsflate.DecompressFrameexplicitly rejects fragmented messages (when FIN ≠ 1) and only works on single-frame payloads. Additionally, per RFC 7692 rules, the RSV1 compression bit may only be set on the first frame of a message; continuation frames must not have RSV1 set.The current code calls
wsflate.DecompressFrameon individual frames without buffering continuation frames, which will:
- Drop continuation frames after the first text/binary frame
- Truncate large/fragmented messages
- Desynchronize the stream if a client sends fragmented compressed messages
To handle fragmented compressed messages correctly, buffer data frames until the FIN bit is set, then decompress the reassembled payload. Alternatively, use the library's recommended streaming approach:
wsutil.Readerwithwsflate.MessageStateas aRecvExtension, wrapped withwsflate.Readerfor streaming decompression.🔧 Suggested buffering of fragmented compressed messages
- if c.compressionEnabled { - // Read frames directly and handle compression - controlHandler := wsutil.ControlFrameHandler(c.conn, ws.StateServerSide) - for { - frame, err := ws.ReadFrame(c.conn) - if err != nil { - return err - } - - // Unmask client frames - if frame.Header.Masked { - ws.Cipher(frame.Payload, frame.Header.Mask, 0) - } - - if frame.Header.OpCode.IsControl() { - if err := controlHandler(frame.Header, bytes.NewReader(frame.Payload)); err != nil { - return err - } - continue - } - - if frame.Header.OpCode == ws.OpText || frame.Header.OpCode == ws.OpBinary { - // Check if frame is compressed (RSV1 bit set) - isCompressed, err := wsflate.IsCompressed(frame.Header) - if err != nil { - return err - } - if isCompressed { - frame, err = wsflate.DecompressFrame(frame) - if err != nil { - return err - } - } - text = frame.Payload - break - } - } - } else { + if c.compressionEnabled { + controlHandler := wsutil.ControlFrameHandler(c.conn, ws.StateServerSide) + var ( + frame ws.Frame + payload []byte + isCompressed bool + op ws.OpCode + started bool + ) + for { + frame, err = ws.ReadFrame(c.conn) + if err != nil { + return err + } + if frame.Header.Masked { + ws.Cipher(frame.Payload, frame.Header.Mask, 0) + } + if frame.Header.OpCode.IsControl() { + if err := controlHandler(frame.Header, bytes.NewReader(frame.Payload)); err != nil { + return err + } + continue + } + if !started { + if frame.Header.OpCode != ws.OpText && frame.Header.OpCode != ws.OpBinary { + continue + } + op = frame.Header.OpCode + started = true + isCompressed, err = wsflate.IsCompressed(frame.Header) + if err != nil { + return err + } + } else if frame.Header.OpCode != ws.OpContinuation { + return fmt.Errorf("unexpected opcode %v while waiting for continuation", frame.Header.OpCode) + } + payload = append(payload, frame.Payload...) + if frame.Header.Fin { + break + } + } + if isCompressed { + frame = ws.NewFrame(op, true, payload) + frame.Header.Rsv = ws.Rsv(true, false, false) + frame, err = wsflate.DecompressFrame(frame) + if err != nil { + return err + } + text = frame.Payload + } else { + text = payload + } + } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/core/websocket.go` around lines 162 - 302, ReadJSON currently calls wsflate.DecompressFrame on single frames, which fails for fragmented compressed messages; update ReadJSON to buffer data frames (accumulating Payloads and respecting FIN) and only call wsflate.DecompressFrame on the reassembled message when FIN is true, OR replace the manual frame loop with the library streaming approach by using wsutil.Reader with wsflate.MessageState as a RecvExtension and wrapping it with wsflate.Reader to read a full decompressed message; also adjust writeCompressed/WriteText/WriteJSON usages if you pick streaming to ensure RSV1 is set only on the first frame and continuation frames are sent without RSV1. Ensure you modify the wsConnectionWrapper.ReadJSON and related write paths (writeCompressed) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@router/core/websocket.go`:
- Around line 162-302: ReadJSON currently calls wsflate.DecompressFrame on
single frames, which fails for fragmented compressed messages; update ReadJSON
to buffer data frames (accumulating Payloads and respecting FIN) and only call
wsflate.DecompressFrame on the reassembled message when FIN is true, OR replace
the manual frame loop with the library streaming approach by using wsutil.Reader
with wsflate.MessageState as a RecvExtension and wrapping it with wsflate.Reader
to read a full decompressed message; also adjust
writeCompressed/WriteText/WriteJSON usages if you pick streaming to ensure RSV1
is set only on the first frame and continuation frames are sent without RSV1.
Ensure you modify the wsConnectionWrapper.ReadJSON and related write paths
(writeCompressed) accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
WEBSOCKETS_COMPRESSION_ENABLEDandWEBSOCKETS_COMPRESSION_LEVEL(1-9) for tuning compression behavior.Tests
Checklist
This is reopening PR #2424 to add support for per-message-deflate to websockets. This is a requirement for us to be using websockets though cosmo-router