Skip to content

Conversation

@jdcormie
Copy link
Member

@jdcormie jdcormie commented Oct 23, 2025

The following BinderServerTransport#start() code was completely untested and did not in fact work:

if (isShutdown()) {
      setState(TransportState.SHUTDOWN_TERMINATED);
      notifyTerminated();
      releaseExecutors();

The problem is that if the listener does in fact arrive via start() after shutdown, BinderTransport's shutdownInternal() has already set the state to SHUTDOWN_TERMINATED (which is not a valid transition from itself). It has also already scheduled a call to notifyTerminated() and releaseExecutors(). This causes a duplicate call to transportTerminated and releasing the same executor twice.

This PR changes start() to leave changing state and releasing executors to shutdownInternal()'s. notifyTerminated() either runs then (if already started) or from within start() (if not yet started)

Fixes #12439.

@jdcormie jdcormie force-pushed the listener-as-future branch 2 times, most recently from 47778b6 to ef64165 Compare October 23, 2025 18:27
@jdcormie jdcormie requested a review from ejona86 October 24, 2025 19:52
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

OkHttp/Netty on server-side don't notify shutdown/termination from the calling thread. They instead trigger operations for the reader and the reader calls the methods. For example, shutdownNow() when still starting will close the connection's file descriptor, which the reading thread will see and notify termination, the same way it does if the client closes the connection. I can see why Binder would be different in this regard.

@jdcormie jdcormie merged commit 91f3f4d into grpc:master Oct 24, 2025
17 checks passed
AgraVator pushed a commit to AgraVator/grpc-java that referenced this pull request Nov 3, 2025
…ase (grpc#12440)

The `isShutdown()` clause of `BinderServerTransport#start()` code was completely
untested and did not in fact work.

The problem is that if the listener does in fact arrive via start()
after shutdown, BinderTransport's `shutdownInternal()` has already set
the state to `SHUTDOWN_TERMINATED` (which is not a valid transition from
itself). It has also already scheduled a call to notifyTerminated() and
releaseExecutors(). This causes a duplicate call to
`transportTerminated` and releasing the same executor twice.

This commit changes `start()` to leave changing state and releasing
executors to `shutdownInternal()`'s. `notifyTerminated()` either runs
then (if already started) or from within `start()` (if not yet started)

Fixes grpc#12439.
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.

binder: BinderServerTransport's shutdown-before-start handling is untested and doesn't actually work

2 participants