Fix Triple flow control deadlock when a message exceeds the stream window - #16443
Open
fudianchn wants to merge 1 commit into
Open
Fix Triple flow control deadlock when a message exceeds the stream window#16443fudianchn wants to merge 1 commit into
fudianchn wants to merge 1 commit into
Conversation
…ndow A single gRPC message larger than the HTTP/2 stream-level flow control window can never complete: LengthFieldStreamingDecoder only reported bytesRead after a full message was assembled, so the partially received bytes were never returned to the peer and both sides waited on each other until the call timed out (issue apache#16427, case 1). Report the ingested bytes in decode() as soon as they are taken off the wire, even if the message they belong to is still incomplete, and drop the per-message report in processBody() to avoid double counting. Also flush the parent channel after consumeBytes() writes a WINDOW_UPDATE: when consumption runs from an executor thread the frame reading loop does not flush it, and flushing the stream channel itself is a no-op for frames written directly to the connection, so the update sat in the outbound buffer and the peer never saw the returned window. Fixes apache#16427 Signed-off-by: 付典 <fudianchn@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16443 +/- ##
============================================
+ Coverage 60.90% 60.94% +0.04%
- Complexity 11765 11766 +1
============================================
Files 1953 1953
Lines 89271 89281 +10
Branches 13473 13474 +1
============================================
+ Hits 54367 54411 +44
+ Misses 29321 29295 -26
+ Partials 5583 5575 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
AI disclosure: this change was prepared with AI coding agents, reviewed and revised line by line by me.
What is the purpose of the change?
Fixes #16427 (case 1).
When a single gRPC message is larger than the HTTP/2 stream-level flow control window, the call deadlocks until it times out: the sender stops once the window is exhausted and waits for WINDOW_UPDATE, while the receiver buffers the partial message and waits for the remaining bytes. Two defects cause this:
LengthFieldStreamingDecoderonly reportedbytesReadafter a full message was assembled (processBody), so bytes buffered inside an incomplete message were never returned to the peer. Nowdecode()reports the ingested bytes as soon as they are taken off the wire, and the per-message report inprocessBody()is dropped to avoid double counting. This matches the grpc-java deframer pattern already referenced in the code comment.consumeBytes()(bothNettyH2StreamChannelon the server side andHttp2TripleClientStreamon the client side) never flushed the WINDOW_UPDATE written by the flow controller: when consumption runs from an executor thread the frame reading loop does not flush it, and flushing the stream channel itself is a no-op for frames written directly to the connection. The frame observed in a frame log sat in the outbound buffer for 5 seconds until a timeout reset flushed it. Both sites now flush the parent channel whenconsumeByteswrote an update.Verified both directions on a real localhost tri connection: with a 64 KiB initial stream window and the ~92 KiB message from the issue, the frame log shows the request and the response each delivered as 65535 + 29003 bytes with WINDOW_UPDATE flowing in between, instead of a deadlock until the 5s deadline. The second failure mode from the issue (window exceeded against a PHP gRPC peer) needs a peer that keeps sending beyond the advertised window and did not reproduce between two Netty endpoints, so it is not addressed here.
Tests:
TripleFlowControlTest(unary and server-stream against a real localhost endpoint, red before the change: DEADLINE_EXCEEDED after 5s, green after),GrpcStreamingDecoderTest.reportsBytesReadForIncompleteMessage(partial message reports its bytes, red before: expected 14 but was 0).mvn -pl dubbo-remoting/dubbo-remoting-http12,dubbo-rpc/dubbo-rpc-triple -am testpasses.Checklist