Repository navigation
Conversation
ejona86
reviewed
Oct 26, 2026
ejona86
left a comment
ejona86
left a comment
Member
There was a problem hiding this comment.
A ClientInbound is created and registered in ongoingCalls by
newStream() but only learns about its ClientStreamListener later, in
ClientStream.start().
Did you consider registering in ongoingCalls within ClientStream.start()? Most real transports do registration in the "transport thread" in start(), and newStream() does nothing more than allocations.
The approach taken here can be fine, but want to make sure you considered the alternative.
jdcormie
force-pushed
the
binder-close-before-start
branch
from
October 27, 2026 04:41
07b613f to
38c097c
Compare
jdcormie
changed the title
jdcormie
force-pushed
the
binder-close-before-start
branch
from
October 27, 2026 04:51
38c097c to
c2b6f44
Compare
BinderClientTransport.newStream() registered a new ClientInbound in ongoingCalls right away but the Inbound only learned about its ClientStreamListener later, in ClientStream.start(). If the transport terminated in between (binderDied(), onUnbound(), a peer's SHUTDOWN_TRANSPORT or shutdownNow(), all of which close ongoing calls asynchronously), closeAbnormal() found an UNINITIALIZED Inbound with no listener or StatsTraceContext to report to. start() then attached the listener to an Inbound that was already closed and the RPC hung forever with its tracers never closed. Registering at newStream() was also a leak waiting to happen: gRPC core can abandon a ClientStream it got from newStream() without ever calling start() or cancel() on it (DelayedStream.setStream() silently drops the real stream if the DelayedStream was cancelled in the meantime). Such a stream stayed in ongoingCalls and counted as in-use forever, so a graceful shutdown() of that transport could never terminate. Make newStream() do allocations only and move registration into start(), as OkHttp and Netty do. start() now initializes the Inbound with its listener and then registers the call under the transport lock so, relative to shutdownInternal()'s snapshot of ongoingCalls, there are only two possible outcomes: either shutdown ran first, in which case the call is never registered and start() reports closed(shutdownStatus, MISCARRIED) (mirroring OkHttp's streamReadyToStart() and allowing a transparent retry on a fresh transport), or we registered an Inbound that already has a listener for the eventual close to be delivered to. That MISCARRIED close is delivered under the Inbound lock alone, like every other close. Because a closed stream may still legally be written to and half-closed, the miscarried stream's Outbound is closed first so that nothing can ever go on the wire for a call the server must never see. Two behaviour changes follow, both consistent with OkHttp/Netty: - transportInUse(true) is reported at start() rather than newStream(). - A graceful shutdown() between newStream() and start() no longer waits for the unstarted stream, whose start() then miscarries. Since nothing can reach an Inbound that was never registered, the only way a client Inbound can still be UNINITIALIZED when cancel() runs is if cancel() came before start(). ClientStream#cancel says that's not allowed and gRPC core's paths honour it (ClientCallImpl arms Context and deadline cancellation only after start(), see issue grpc#1343), so cancel() now throws IllegalStateException instead of silently closing a stream nobody is listening to. A cancel() after a miscarried start() remains a legal no-op. unregisterInbound() only decrements the in-use count for an Inbound that was actually registered.
Inbound.onTransportReady() unconditionally forwarded onReady() to the listener, so flow control unblocking concurrently with a close could invoke onReady() after closed(), which must be the listener's final callback. Only skip the listener, not the Outbound: a server stream's close may still be sitting in its Outbound waiting for the transmit window to open.
jdcormie
force-pushed
the
binder-close-before-start
branch
from
October 27, 2026 05:30
c2b6f44 to
c6cd8f5
Compare
Member
Author
No but that's a good idea! And it seems at least one place in core assumes this. PTAL. |
This branch has not been deployed
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.
See each commit description - will not squash.