binder: Avoid force-terminating BinderTransport on stale unregisterCall task

When an ongoing call finishes and `unregisterCall()` observes
`ongoingCalls.isEmpty()`, it schedules a task on
`scheduledExecutorService` to finish transport shutdown if the transport
is in `TransportState.SHUTDOWN`. However, `unregisterCall()` runs
without holding `synchronized (this)` and may have scheduled this task
while the transport is still in `TransportState.READY`. If a new stream
is created before the transport enters `TransportState.SHUTDOWN`, the
previously scheduled task can execute after `shutdown()` and call
`shutdownInternal(shutdownStatus, true)`, force-terminating the newly
started call with `UNAVAILABLE: Channel shutdown invoked`.

Pass `forceTerminate = false` to `shutdownInternal()` so it re-checks
`ongoingCalls.isEmpty()` while holding `synchronized (this)`.

Author: jdcormie

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);
               }
             }
           });

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();
+  }
 }