Skip to content

client: stop the client-stream transport task when the call is abandoned#226

Merged
iainmcgin merged 2 commits into
connectrpc:mainfrom
EffortlessSteven:claude/cr-0007-client-stream-detach-upstream
Jul 9, 2026
Merged

client: stop the client-stream transport task when the call is abandoned#226
iainmcgin merged 2 commits into
connectrpc:mainfrom
EffortlessSteven:claude/cr-0007-client-stream-detach-upstream

Conversation

@EffortlessSteven

@EffortlessSteven EffortlessSteven commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What this does

Abandoning a call_client_stream call (dropping its future or hitting the deadline) while the transport is still awaiting response headers leaves the background transport.send(...) running indefinitely: the detached task driving it only checks the receiver after the send resolves.

Fix: the spawned task now stops the send when the caller stops waiting (future dropped or deadline fired). The response and transport-error paths are unchanged.

Note that this is also a behavior change, not only a leak fix: abandonment now cancels the underlying request, so a request that was still being sent may never reach the server. The # Cancellation docs and the changelog entry call this out, matching the equivalent BidiStream documentation from #221.

Verification

Unpatched, the abandonment tests time out: the transport send future is never dropped.

Check Result
abandon: deadline / cancel / mid-drain unpatched: send never dropped; patched: dropped
success, transport-error paths unchanged
fmt, clippy -D warnings, test clean / 562 passed

Review map

  • call_client_stream: the spawned send task selects transport.send(...) against resp_tx.closed(), so a dropped receiver (cancellation or deadline) stops the send instead of leaving it detached.
  • connectrpc/Cargo.toml: the always-compiled call_client_stream now uses tokio::select!, so the base tokio dep gains macros; the server feature's own tokio/macros entry is dropped as redundant, with a note on the base dependency explaining why macros is load-bearing.
  • tests: pin both abandonment paths dropping the send future (with mid-drain backpressure), and preserve the success and transport-error paths.
  • maintainer fixup (6f67559): documents the delivery caveat in the rustdoc and changelog, adds a # Cancellation note to call_server_stream, and consolidates the tokio/macros feature declaration.

Fixes #224

EffortlessSteven and others added 2 commits July 9, 2026 07:35
`call_client_stream` runs `transport.send(...)` in a detached task and
bridges the result back through a oneshot, then drains the request
iterator and awaits the receiver. The task awaited the send to completion
before checking the receiver, so if the call future was dropped (caller
cancellation) or its deadline fired while the transport was still waiting
for response headers, the receiver was dropped but the detached task kept
polling `transport.send(...)` indefinitely.

Race the send against the receiver closing inside the spawned task, so
caller cancellation or deadline expiry stops polling the transport send
and drops it. The normal response and transport-error paths still send
their result through the oneshot unchanged.

Refs connectrpc#224.

Signed-off-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
Document that abandoning a streaming call cancels the underlying request
(a request still being sent may never reach the server) in the
call_client_stream cancellation section and the changelog entry, and add
the matching cancellation note to call_server_stream. Drop the server
feature's now-redundant tokio/macros entry in favour of a note on the
base dependency explaining why macros is load-bearing there.

No-Verification-Needed: docs, comments, and build metadata only; no runtime surface
Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>

@iainmcgin iainmcgin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[claude code] Reviewed on Iain's behalf; approving.

Verification performed locally:

  • Reverted just the fix (keeping the new tests) and confirmed all three abandonment tests fail unpatched and pass patched — they genuinely gate the bug.
  • Confirmed the ownership invariant the fix relies on: resp_rx lives inside the timeout_at-wrapped block, so both abandonment paths (future drop, deadline expiry) drop it, and the spawned task's closed() branch then drops the in-flight send. The simultaneous-readiness race is benign in both directions.
  • Confirmed this was the last detached transport-send spawn in the client (call_bidi_stream keeps its JoinHandle per #221; call_server_stream awaits the send inline), closing out the #224 leak class.

I pushed a small maintainer fixup (6f67559) rather than round-tripping review comments: it documents the caller-visible consequence (an abandoned call now cancels the underlying request, so a request still being sent may never reach the server — the same caveat #221 documented for BidiStream), adds the matching # Cancellation note to call_server_stream, consolidates the now-redundant server-feature tokio/macros entry into a note on the base dependency, and rewords a few test comments into code-present tense.

Thanks for the contribution — the started_rx gate proving the drop is of an in-flight (not merely unpolled) future is a nice touch, and the mid-drain backpressure case is exactly the coverage this needed.

@iainmcgin
iainmcgin enabled auto-merge July 9, 2026 21:50
@iainmcgin
iainmcgin added this pull request to the merge queue Jul 9, 2026
Merged via the queue into connectrpc:main with commit c49dbd4 Jul 9, 2026
14 checks passed
iainmcgin added a commit that referenced this pull request Jul 12, 2026
#226 fixed the channel-era client-stream leak by racing the detached
send task against receiver closure. The stream-as-body design has no
detached task — dropping the call future drops the transport send and
the request body directly — so the mechanism is superseded while the
guarantees are kept:

- the # Cancellation rustdoc sections are ported (client-stream text
  adapted to the direct-drop mechanism; server-stream text unchanged)
- all five #226 tests are ported to the stream API; the mid-drain
  backpressure case becomes an unread-request-body case, since there
  is no drain loop or channel to park in
- tokio/macros stays on the base dependency per #226's consolidation,
  with the comment narrowed to the server accept loop now that the
  client-stream path no longer uses select!
- the #226 changelog fragment is reworded to describe the behavior
  (abandonment stops the in-flight send) rather than the superseded
  background-task mechanism

Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
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.

client: call_client_stream detaches its transport-send task — same leak class as #221

2 participants