[ISSUE #16399] Close late input after DefaultStreamingDecoder shutdown - #16406
Open
cyforkk wants to merge 2 commits into
Open
[ISSUE #16399] Close late input after DefaultStreamingDecoder shutdown#16406cyforkk wants to merge 2 commits into
cyforkk wants to merge 2 commits into
Conversation
…hutdown Mirror the apache#16391 fix: when decode() is called after the decoder has been closed (via close() or onStreamClosed()), close the input stream instead of ignoring it. This prevents ByteBuf leaks in the non-gRPC Triple HTTP/2 streaming path when late DATA frames arrive after the decoder has started closing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16406 +/- ##
============================================
+ Coverage 60.87% 60.91% +0.03%
+ Complexity 11766 11761 -5
============================================
Files 1953 1953
Lines 89273 89277 +4
Branches 13473 13473
============================================
+ Hits 54346 54382 +36
+ Misses 29333 29303 -30
+ Partials 5594 5592 -2
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:
|
Cover the IOException -> DecodeException branch to improve patch coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
|
@LI123456mo Thanks for the review and approval! Since this PR requires 2 approving reviews, could you please help ping another maintainer to take a look when they have time? |
Contributor
@cyforkk , for sure they will see , sometimes maybe they are just busy but they will |
Author
|
@LI123456mo Alright bro, have a good one. |
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
3 participants
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.
Fixes #16399
What is the purpose of the change
DefaultStreamingDecoder.decode()does not close the input stream when the decoder has already been closed (closed == true). In the Triple HTTP/2 path, DATA frames are wrapped inByteBufInputStream(content, true)(i.e.,releaseOnClose = true), meaning the underlying pooledByteBufis released only whenInputStream.close()is called. If a late DATA frame arrives asynchronously after the decoder has started closing, theByteBufis never released, causing a leak.This is the exact same bug pattern as #16389, which was fixed by #16391 for
LengthFieldStreamingDecoder. This PR mirrors that fix forDefaultStreamingDecoder.Brief changelog
decode()when the decoder is closed.DecodeException.close()andonStreamClosed()decoder states.Verifying this change
DefaultStreamingDecoderTest: 2 tests passed.LengthFieldStreamingDecoderTest: 2 tests passed.dubbo-remoting-http12tests: 23 tests passed, 2 skipped.