fix: release late response ByteBuf when callback executor is shut down (#16415) - #16418
Open
jamespud wants to merge 3 commits into
Open
fix: release late response ByteBuf when callback executor is shut down (#16415)#16418jamespud wants to merge 3 commits into
jamespud wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16418 +/- ##
=========================================
Coverage 60.87% 60.88%
- Complexity 11766 11768 +2
=========================================
Files 1953 1953
Lines 89273 89280 +7
Branches 13473 13474 +1
=========================================
+ Hits 54346 54357 +11
+ Misses 29333 29330 -3
+ Partials 5594 5593 -1
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:
|
apache#16415) When a sync Triple call times out, AsyncRpcResult#get(timeout) shuts down the per-call ThreadlessExecutor in a finally block. Late DATA frames that arrive afterwards are submitted via SerializingExecutor, which silently drops tasks submitted to a shut-down executor, so doOnData never runs and the DATA frame's ByteBuf is never released - a Netty ByteBuf leak that grows on every timed-out call with in-flight response data. Check the callback executor's shutdown state in onData and release the ByteBuf immediately when it is shut down, instead of relying on the submission being rejected (SerializingExecutor swallows the rejection on shutdown). Verification: - TripleClientStreamTest#testOnDataReleaseByteBufAfterCallbackExecutorShutdown (new): a DATA-frame ByteBuf received after the callback executor is shut down is released (refCnt == 0). This test fails before the fix (refCnt == 1). Fixes apache#16415
jamespud
force-pushed
the
feature/fix-16415-triple-bytebuf-leak
branch
from
August 11, 2026 05:57
627725e to
f3afd83
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Netty ByteBuf leak in the Triple client stream when late DATA frames arrive after a sync call times out and the per-call callback executor has already been shut down, causing the decode task submission to be dropped.
Changes:
- Track the original (raw) callback executor in
AbstractStreamand exposeisCallbackExecutorShutdown(). - In
AbstractTripleClientStream.onData(), immediately release late DATAByteBufs when the callback executor is shut down (instead of submitting a task that will be dropped). - Add a unit test to assert late DATA
ByteBufs are released after callback-executor shutdown.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/stream/AbstractStream.java | Retains the raw callback executor and provides shutdown-state detection for leak prevention. |
| dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/stream/AbstractTripleClientStream.java | Releases late response ByteBufs when tasks would be dropped due to executor shutdown/rejection; adjusts logging. |
| dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/protocol/tri/stream/TripleClientStreamTest.java | Adds a regression test ensuring late DATA ByteBuf is released after callback executor shutdown. |
Suppressed comments (1)
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/stream/AbstractTripleClientStream.java:585
- The WARN message in the executor-rejection path says "Drop late response data", but this catch block can also be triggered by thread pool saturation (as noted in the comment). The log message should not imply the data is necessarily late; otherwise it can mislead debugging/alert triage.
// Tasks will be rejected when the thread pool is closed or full,
// ByteBuf needs to be released to avoid out of heap memory leakage.
// For example, ThreadLessExecutor will be shutdown when request timeout {@link AsyncRpcResult}
ReferenceCountUtil.release(data);
LOGGER.warn(PROTOCOL_FAILED_RESPONSE, "", "", "Drop late response data, executor rejected the task", t);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+150
to
+155
| Http2StreamChannel http2StreamChannel = mock(Http2StreamChannel.class); | ||
| when(http2StreamChannel.isActive()).thenReturn(true); | ||
| when(http2StreamChannel.newSucceededFuture()).thenReturn(channel.newSucceededFuture()); | ||
| when(http2StreamChannel.eventLoop()).thenReturn(new NioEventLoopGroup().next()); | ||
| when(http2StreamChannel.newPromise()).thenReturn(channel.newPromise()); | ||
| when(http2StreamChannel.parent()).thenReturn(channel); |
Comment on lines
+569
to
+575
| // The callback executor (e.g. ThreadlessExecutor) has been shut down, e.g. | ||
| // after the request timed out {@link AsyncRpcResult}. SerializingExecutor would | ||
| // silently drop the submitted task so doOnData would never run; release the | ||
| // ByteBuf now to avoid out of heap memory leakage. | ||
| ReferenceCountUtil.release(data); | ||
| LOGGER.warn(PROTOCOL_FAILED_RESPONSE, "", "", "Drop late response data, callback executor is shutdown"); | ||
| return; |
Address Copilot review feedback on apache#16418: - Rephrase onData comments and WARN messages so they do not imply the response data is always late (the rejection can also come from thread pool saturation) and drop the inaccurate 'out of heap memory' wording (ByteBufs can be off-heap). - Reuse the EmbeddedChannel event loop in TripleClientStreamTest instead of creating a new NioEventLoopGroup, which started threads that were never shut down.
Contributor
Author
|
@zrlw Hi, the "Unit Test" jobs are failing only on the pre-existing flaky SpringBootConfigPropsTest (expected: but was: , test-isolation issue in dubbo-config-spring; it passes locally and is unrelated to this PR). Could you re-run the failed unit-test jobs? Thanks! |
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.
What is the purpose of the change
Fixes #16415.
When a sync Triple call times out,
AsyncRpcResult#get(timeout)shuts down the per-callThreadlessExecutorin afinallyblock. Any DATA frame that arrives afterwards is handed toAbstractTripleClientStream.ClientTransportListener.onData(), which submits the decode task viaSerializingExecutor. Because the underlying executor is already shut down,SerializingExecutorsilently drops the submitted task, sodoOnData()never runs and the DATA frame'sByteBuf(wrapped inByteBufInputStream(data, true)only insidedoOnData) is never released — a Netty ByteBuf leak that grows on every timed-out call with in-flight response data.Root cause
For a sync Triple call the callback executor is a per-call
ThreadlessExecutor. On timeout,AsyncRpcResult#get(timeout)shuts it down in afinallyblock. Late DATA frames are then submitted to aSerializingExecutorwrapping that shut-down executor;SerializingExecutorsilently drops them (a behavior from #15122), sodoOnData()never executes and the ByteBuf is never released.Brief changelog
AbstractStream(dubbo-rpc-triple): retain the raw callback executor and exposeisCallbackExecutorShutdown().AbstractTripleClientStream(dubbo-rpc-triple): inonData, when the callback executor has been shut down, release the DATA frame'sByteBufimmediately instead of submitting a task that would be silently dropped; log at WARN since dropping late data after a timeout is expected.Verifying the change
TripleClientStreamTest#testOnDataReleaseByteBufAfterCallbackExecutorShutdown(new): a DATA-frame ByteBuf received after the callback executor is shut down is released (refCnt == 0). This test fails before the fix (refCnt == 1).Fixes #16415