Skip to content

feat(streaming): support max receive message size foor gRPC and TTHea… - #1980

Open
junliurs wants to merge 2 commits into
cloudwego:mainfrom
junliurs:fix/streaming-max-receive-message-size
Open

feat(streaming): support max receive message size foor gRPC and TTHea…#1980
junliurs wants to merge 2 commits into
cloudwego:mainfrom
junliurs:fix/streaming-max-receive-message-size

Conversation

@junliurs

Copy link
Copy Markdown
Contributor

…der streaming

What type of PR is this?

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en:
zh(optional):

(Optional) Which issue(s) this PR fixes:

(optional) The PR that updates user documentation:

@junliurs
junliurs requested review from a team as code owners July 29, 2026 06:21
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.23%. Comparing base (8bb270e) to head (97f2d30).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1980      +/-   ##
==========================================
+ Coverage   62.95%   63.23%   +0.27%     
==========================================
  Files         394      396       +2     
  Lines       30267    30415     +148     
==========================================
+ Hits        19056    19232     +176     
+ Misses       9924     9895      -29     
- Partials     1287     1288       +1     
Flag Coverage Δ
integration 51.85% <39.50%> (-0.03%) ⬇️
unit 53.93% <98.76%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@junliurs
junliurs force-pushed the fix/streaming-max-receive-message-size branch from 3e88db8 to 05a6fb7 Compare July 29, 2026 07:15
@ccoalm

ccoalm commented Aug 4, 2026

Copy link
Copy Markdown

Drive-by review from a downstream user — we hit exactly this gap last week and independently
concluded the fix had to live here, so this PR is very welcome. Two notes, one of which I think
is a real bug.

Context (why this matters): we run a streaming LLM gateway on Kitex gRPC (nphttp2) that now
carries inline image bytes in the request. There is currently no way to bound an inbound message:
decodeGRPCFrame reads the 5-byte prefix and passes dLen straight into in.Next(dLen), and
nphttp2/buffer.go growRbuf then does mcache.Malloc(0, dLen) on a peer-declared length.
Our application-level size gates run after a full decode, so they cannot prevent the buffering.
server.WithCodec does not reach the nphttp2 handler (the codec is constructed inline at
server_handler.go:70), so this genuinely is not fixable downstream.

The placement is right. Putting the check before in.Next(dLen) also prevents the
speculative allocation, not just the buffering — worth keeping exactly there in any refactor.


1. The status code looks wrong. The check returns
perrors.NewProtocolErrorWithType(perrors.SizeLimit, ...). As far as I can trace on v0.16.2,
protocolException is not a *remote.TransError, does not implement GRPCStatus(), and is not
in kitexErrConvTab (pkg/remote/trans/nphttp2/codes.go:30-35), so convertStatus falls through
to status.New(codes.Internal, err.Error()).

grpc-go returns codes.ResourceExhausted when a message exceeds the max receive size. Internal
tells the caller "the server broke", when the truth is "your message was too large" — a
caller-actionable, non-retryable-as-is condition that clients typically handle by chunking. It
also collides with genuine server faults in dashboards and alerting.

Would you consider mapping this to ResourceExhausted? kerrors.ErrOverlimit is already mapped
that way, so either reusing it or adding a perrors.SizeLimit entry to the conversion table
would do it. Happy to be corrected if the error takes a different path than I traced.

2. 0 means unlimited — is that the intended default? The guard is
maxReceiveMessageSize > 0 && dLen > maxReceiveMessageSize, and I don't see a default constant,
so merging this leaves every existing server unprotected until it opts in. That may well be the
deliberate choice for backward compatibility — if so, could the option's doc comment state it
explicitly? Users coming from grpc-go will assume a 4MB default is in effect (that assumption is
already widespread, including in search results and blog posts about Kitex), and silently getting
"unlimited" instead is the more dangerous direction to be wrong in.

Thanks for putting this together — happy to test it against our workload if that helps it land.

@junliurs

junliurs commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Drive-by review from a downstream user — we hit exactly this gap last week and independently concluded the fix had to live here, so this PR is very welcome. Two notes, one of which I think is a real bug.

Context (why this matters): we run a streaming LLM gateway on Kitex gRPC (nphttp2) that now carries inline image bytes in the request. There is currently no way to bound an inbound message: decodeGRPCFrame reads the 5-byte prefix and passes dLen straight into in.Next(dLen), and nphttp2/buffer.go growRbuf then does mcache.Malloc(0, dLen) on a peer-declared length. Our application-level size gates run after a full decode, so they cannot prevent the buffering. server.WithCodec does not reach the nphttp2 handler (the codec is constructed inline at server_handler.go:70), so this genuinely is not fixable downstream.

The placement is right. Putting the check before in.Next(dLen) also prevents the speculative allocation, not just the buffering — worth keeping exactly there in any refactor.

1. The status code looks wrong. The check returns perrors.NewProtocolErrorWithType(perrors.SizeLimit, ...). As far as I can trace on v0.16.2, protocolException is not a *remote.TransError, does not implement GRPCStatus(), and is not in kitexErrConvTab (pkg/remote/trans/nphttp2/codes.go:30-35), so convertStatus falls through to status.New(codes.Internal, err.Error()).

grpc-go returns codes.ResourceExhausted when a message exceeds the max receive size. Internal tells the caller "the server broke", when the truth is "your message was too large" — a caller-actionable, non-retryable-as-is condition that clients typically handle by chunking. It also collides with genuine server faults in dashboards and alerting.

Would you consider mapping this to ResourceExhausted? kerrors.ErrOverlimit is already mapped that way, so either reusing it or adding a perrors.SizeLimit entry to the conversion table would do it. Happy to be corrected if the error takes a different path than I traced.

2. 0 means unlimited — is that the intended default? The guard is maxReceiveMessageSize > 0 && dLen > maxReceiveMessageSize, and I don't see a default constant, so merging this leaves every existing server unprotected until it opts in. That may well be the deliberate choice for backward compatibility — if so, could the option's doc comment state it explicitly? Users coming from grpc-go will assume a 4MB default is in effect (that assumption is already widespread, including in search results and blog posts about Kitex), and silently getting "unlimited" instead is the more dangerous direction to be wrong in.

Thanks for putting this together — happy to test it against our workload if that helps it land.

@ccoalm
Thanks for the detailed review and for tracing the allocation and error-conversion paths.

You are right about the status code. decodeGRPCFrame returned a
perrors.SizeLimit, but protocolException neither implements GRPCStatus()
nor matches any entry in kitexErrConvTab, so the server-side
convertStatus path fell back to codes.Internal.

I changed the size-limit path in decodeGRPCFrame to return
kerrors.ErrOverlimit with the detailed length error as its cause. Kitex
already maps ErrOverlimit to codes.ResourceExhausted, so an oversized
message now produces the expected gRPC status without changing the handling
of unrelated protocol errors. The codec regression test now verifies the new
error classification with errors.Is(err, kerrors.ErrOverlimit).

For the default, unlimited is intentional for backward compatibility. Changing
the default to grpc-go's 4 MiB limit would reject messages that existing Kitex
services currently accept, so this PR keeps the limit opt-in. I agree that this
was not explicit enough: the public client/server options and the lower-level
codec option now state that the default is unlimited for backward compatibility
and that non-positive values preserve that behavior.

The size check remains before in.Next(dLen), so an oversized peer-declared
length is rejected before the speculative allocation.

Thanks again — both points were helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants