Skip to content

stub: drain pending reads on cancel to release their buffers - #13009

Open
fudianchn wants to merge 1 commit into
grpc:masterfrom
fudianchn:fix/stub-cancel-drain-pending-reads
Open

stub: drain pending reads on cancel to release their buffers#13009
fudianchn wants to merge 1 commit into
grpc:masterfrom
fudianchn:fix/stub-cancel-drain-pending-reads

Conversation

@fudianchn

@fudianchn fudianchn commented Aug 28, 2026

Copy link
Copy Markdown

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's ByteBufs, as reported in #12355.

How

cancel() now calls executor.drain() after call.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

  • New BlockingClientCallTest#testCancelDrainsPendingReads: fails without the change (executor queue non-empty after cancel), passes with it.
  • Issue reproducer (manualflowcontrol example server + v2 blocking client doing write/read/write/sleep 1s/cancel, 10 iterations, -Dio.netty.leakDetection.level=paranoid -Dio.netty.leakDetection.targetRecords=100): master reports 1 LEAK: 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:

  • master: 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.
  • this change: no leak reports, client exits cleanly.

Fixes #12355

@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 28, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: fudianchn / name: 付典 (e8e3c69)

@fudianchn
fudianchn marked this pull request as draft August 28, 2026 04:44
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>
@fudianchn
fudianchn force-pushed the fix/stub-cancel-drain-pending-reads branch from 4981427 to e8e3c69 Compare August 28, 2026 04:45
@fudianchn
fudianchn marked this pull request as ready for review August 28, 2026 04:45
@fudianchn

Copy link
Copy Markdown
Author

The failing bazel job is unrelated to this change. Dependency resolution fails before any compilation with:

ERROR: Error computing the main repository mapping: Yanked version detected in your resolved dependency graph: protobuf@36.0, for the reason: macOS prebuilt toolchain integrity mismatch (protocolbuffers/protobuf#29313). Use 36.0.bcr.1 instead.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BlockingClientCall.cancel may orphan and leak pending reads

1 participant