Skip sendMaxBytes check for EndStream control frames (#907)#908
Open
washanhanzi wants to merge 1 commit into
Open
Skip sendMaxBytes check for EndStream control frames (#907)#908washanhanzi wants to merge 1 commit into
washanhanzi wants to merge 1 commit into
Conversation
Member
|
I don't think this is the correct approach. The "max send size" is there for a reason, to limit buffering/memory usage to read an envelope in the recipient. If the recipient has a similar max receive size, they will reject this frame. I think a better approach is to fallback to a minimal end-stream frame if the normal end-stream frame is too large. If this minimal end-frame is still too large, then we could send it anyway since a minimal end-stream frame with a useful error message would probably be no more than 100 bytes (so if the max send and receive sizes are set that low, shame on whoever set them 😝). |
Instead of unconditionally bypassing the sendMaxBytes check for control
frames, implement a graduated fallback strategy in envelopeWriter:
1. Try to send the full EndStream frame (error code + message + details).
2. If it exceeds sendMaxBytes, call a reduce closure to strip error
details in-place, re-marshal via a marshal closure, and retry.
3. If even the minimal frame is too large (unreasonably low limit),
force-send it anyway.
The fallback logic lives entirely in envelope.go's writeControlFrame,
which takes two closures from the protocol layer:
- marshal: serializes the current frame state into bytes. Because it
closes over the frame struct, it automatically reflects any mutations
made by reduce.
- reduce: mutates the captured frame struct to strip non-essential data
(error details for Connect, grpc-status-details-bin for gRPC-Web).
This keeps sendMaxBytes enforcement intact for control frames while
guaranteeing that the client always receives at least a minimal error.
Fixes connectrpc#907
Signed-off-by: jingyu <francismajere@gmail.com>
Author
|
Done. Also updated the PR comment. Now use two-stage fallback for oversized EndStream control frames. |
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
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.
Use two-stage fallback for oversized EndStream control frames
Instead of unconditionally bypassing the sendMaxBytes check for control
frames, implement a graduated fallback strategy in envelopeWriter:
details in-place, re-marshal via a marshal closure, and retry.
force-send it anyway.
The fallback logic lives entirely in envelope.go's writeControlFrame,
which takes two closures from the protocol layer:
closes over the frame struct, it automatically reflects any mutations
made by reduce.
(error details for Connect, grpc-status-details-bin for gRPC-Web).
This keeps sendMaxBytes enforcement intact for control frames while
guaranteeing that the client always receives at least a minimal error.
Fixes #907