diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index cfb55f5f72a..db06d3052ab 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -403,13 +403,15 @@ protected void unregisterInbound(Inbound inbound) { final void unregisterCall(int callId) { boolean removed = (ongoingCalls.remove(callId) != null); if (removed && ongoingCalls.isEmpty()) { - // Possibly shutdown (not synchronously, since inbound is held). + // If inState(SHUTDOWN), we'd need to terminate (as the last call to finish). But we can't + // even check our state safely right now because inbound/outbound is held. Instead, do the + // check later on a clean stack. scheduledExecutorService.execute( () -> { synchronized (this) { if (inState(TransportState.SHUTDOWN)) { - // No more ongoing calls, and we're shutdown. Finish the shutdown. - shutdownInternal(shutdownStatus, true); + // Don't forceTerminate because ongoingCalls may no longer be empty. + shutdownInternal(shutdownStatus, /* forceTerminate= */ false); } } }); diff --git a/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java b/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java index a8fbec75a5b..6556a83de1e 100644 --- a/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java +++ b/binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java @@ -17,18 +17,23 @@ package io.grpc.binder.internal; import static com.google.common.truth.Truth.assertThat; +import static io.grpc.binder.internal.BinderTransport.FIRST_CALL_ID; +import static io.grpc.binder.internal.TransactionBuilder.newOutOfBandCloseTxnBuilder; +import static io.grpc.binder.internal.TransactionBuilder.newStreamTxnToServerBuilder; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.doThrow; import static org.robolectric.Shadows.shadowOf; +import android.os.Binder; import android.os.IBinder; import android.os.Looper; import android.os.Parcel; import android.os.RemoteException; import com.google.common.collect.ImmutableList; import io.grpc.Attributes; +import io.grpc.Metadata; import io.grpc.Status; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.MockServerTransportListener; @@ -122,4 +127,33 @@ public void testStartAfterShutdownNoIdle() throws Exception { assertThat(transportListener.isTerminated()).isTrue(); } + + @Test + public void testShutdownWhileCallActiveAndPreviousUnregisterPending() throws Exception { + transport = newBinderServerTransportBuilder().setCallbackBinder(new Binder()).build(); + transport.start(transportListener); + + // Start and finish call 1 while the transport is READY. This schedules a task on the executor + // to check whether the transport should terminate if it is in the SHUTDOWN state. + newStreamTxnToServerBuilder(0) + .withPrefix("service/Method", new Metadata()) + .dispatchTo(transport, FIRST_CALL_ID); + newOutOfBandCloseTxnBuilder(Status.CANCELLED).dispatchTo(transport, FIRST_CALL_ID); + + // Before that scheduled task runs, start call 2 and initiate a graceful shutdown. + newStreamTxnToServerBuilder(0) + .withPrefix("service/Method", new Metadata()) + .dispatchTo(transport, FIRST_CALL_ID + 1); + transport.shutdown(); + + // Run the pending task from call 1's unregistration. Because call 2 is still active, the + // transport must not terminate yet. + shadowOf(Looper.getMainLooper()).idle(); + assertThat(transportListener.isTerminated()).isFalse(); + + // Once call 2 finishes, the transport terminates. + newOutOfBandCloseTxnBuilder(Status.CANCELLED).dispatchTo(transport, FIRST_CALL_ID + 1); + shadowOf(Looper.getMainLooper()).idle(); + assertThat(transportListener.isTerminated()).isTrue(); + } }