stub: drain pending reads on cancel to release their buffers - #13009
stub: drain pending reads on cancel to release their buffers#13009fudianchn wants to merge 1 commit into
Conversation
|
|
BlockingClientCall.cancel() stopped all reads and writes without draining the call executor, orphaning read-delivery tasks already queued by the transport. Those tasks hold the message buffers, which are only released when the task runs and observes the cancelled stream, so cancelling a call with an undelivered read leaked the transport's ByteBufs. Drain the executor in cancel() so the queued tasks run and their buffers are released. Fixes grpc#12355 Signed-off-by: 付典 <fudianchn@gmail.com>
4981427 to
e8e3c69
Compare
|
The failing bazel job is unrelated to this change. Dependency resolution fails before any compilation with:
MODULE.bazel still pins 36.0 (from #12970), and the same bazel job has been failing on master since Aug 26. The Gradle jobs that build and test this change all pass. A MODULE.bazel bump to 36.0.bcr.1 fixes it; I can send that separately if useful. |
|
FYI, there is an existing PR (#12372) from last year which fixes this issue and is still waiting for review. |
2 participants
AI disclosure: this change was prepared with AI coding agents, reviewed and revised line by line by me.
What
Drain the call executor in
BlockingClientCall.cancel()so read-delivery tasks already queued by the transport are processed instead of orphaned.Why
cancel()is the documented way to abandon a v2 blocking call. It stopped all reads and writes but never drained the executor, so a read that had been received but not yet delivered kept its delivery task queued forever. The task holds the message's transport buffers, which are only released when the task runs and observes the cancelled stream (the delivery path closes the producer when the call has an error status). Cancelling with an undelivered read therefore leaks the transport'sByteBufs, as reported in #12355.How
cancel()now callsexecutor.drain()aftercall.cancel(...). Each queued task then runs either the closed-stream discard path, which releases its buffers, or, if the cancel has not fully propagated yet, delivers the message into the buffer, consistent with the existing "some reads that are in flight may still happen after the cancel" behavior.Limitation: abandoning a call without
cancel()still leaves queued tasks undrained, matching the long-standing v1 iterator caveat ("the iterator can result in leaks if not completely consumed").Root cause
The executor of a v2 blocking call is driven only by the user's read/write calls.
cancel()was the one exit that stopped driving it, so tasks queued before the cancel were never run.Testing
BlockingClientCallTest#testCancelDrainsPendingReads: fails without the change (executor queue non-empty after cancel), passes with it.-Dio.netty.leakDetection.level=paranoid -Dio.netty.leakDetection.targetRecords=100): master reports 1LEAK: ByteBuf.release() was not called; this change reports none, stable across 3 runs.Verification of the original issue
Same reproducer as the issue, run on master and on this branch:
SEVERE: LEAK: ByteBuf.release() was not called before it's garbage-collected, with the leaked buffer's access records showing the deframed message held by the queued delivery task.Fixes #12355