Reject further SETUP_TRANSPORT requests post-BinderServer shutdown (#11260)

Fixes #8931

Author: jdcormie

binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java

@@ -122,11 +122,6 @@ public void socketStats() throws Exception {}
   @Override
   public void flowControlPushBack() throws Exception {}
 
-  @Test
-  @Ignore("Not yet implemented. See https://github.com/grpc/grpc-java/issues/8931")
-  @Override
-  public void serverNotListening() throws Exception {}
-
   @Test
   @Ignore("This test isn't appropriate for BinderTransport.")
   @Override

binder/src/androidTest/java/io/grpc/binder/internal/LeakSafeOneWayBinderTest.java

@@ -21,6 +21,7 @@
 
 import android.os.Parcel;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
+import io.grpc.binder.internal.LeakSafeOneWayBinder.TransactionHandler;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -29,28 +30,34 @@
 public final class LeakSafeOneWayBinderTest {
 
   private LeakSafeOneWayBinder binder;
+  private final FakeHandler handler = new FakeHandler();
 
-  private int transactionsHandled;
-  private int lastCode;
-  private Parcel lastParcel;
+  static class FakeHandler implements TransactionHandler {
+    int transactionsHandled;
+    int lastCode;
+    Parcel lastParcel;
 
-  @Before
-  public void setUp() {
-    binder = new LeakSafeOneWayBinder((code, parcel) -> {
+    @Override
+    public boolean handleTransaction(int code, Parcel parcel) {
       transactionsHandled++;
       lastCode = code;
       lastParcel = parcel;
       return true;
-    });
+    }
+  }
+
+  @Before
+  public void setUp() {
+    binder = new LeakSafeOneWayBinder(handler);
   }
 
   @Test
   public void testTransaction() {
     Parcel p = Parcel.obtain();
     assertThat(binder.onTransact(123, p, null, FLAG_ONEWAY)).isTrue();
-    assertThat(transactionsHandled).isEqualTo(1);
-    assertThat(lastCode).isEqualTo(123);
-    assertThat(lastParcel).isSameInstanceAs(p);
+    assertThat(handler.transactionsHandled).isEqualTo(1);
+    assertThat(handler.lastCode).isEqualTo(123);
+    assertThat(handler.lastParcel).isSameInstanceAs(p);
     p.recycle();
   }
 
@@ -59,7 +66,7 @@ public void testDropsTwoWayTransactions() {
     Parcel p = Parcel.obtain();
     Parcel reply = Parcel.obtain();
     assertThat(binder.onTransact(123, p, reply, 0)).isFalse();
-    assertThat(transactionsHandled).isEqualTo(0);
+    assertThat(handler.transactionsHandled).isEqualTo(0);
     p.recycle();
     reply.recycle();
   }
@@ -71,7 +78,21 @@ public void testDetach() {
     assertThat(binder.onTransact(456, p, null, FLAG_ONEWAY)).isFalse();
 
     // The transaction shouldn't have been processed.
-    assertThat(transactionsHandled).isEqualTo(0);
+    assertThat(handler.transactionsHandled).isEqualTo(0);
+
+    p.recycle();
+  }
+
+  @Test
+  public void testReplace() {
+    binder = new LeakSafeOneWayBinder(handler);
+    Parcel p = Parcel.obtain();
+    FakeHandler handler2 = new FakeHandler();
+    binder.setHandler(handler2);
+    assertThat(binder.onTransact(456, p, null, FLAG_ONEWAY)).isTrue();
+
+    assertThat(handler.transactionsHandled).isEqualTo(0);
+    assertThat(handler2.transactionsHandled).isEqualTo(1);
 
     p.recycle();
   }
@@ -81,9 +102,9 @@ public void testMultipleTransactions() {
     Parcel p = Parcel.obtain();
     assertThat(binder.onTransact(123, p, null, FLAG_ONEWAY)).isTrue();
     assertThat(binder.onTransact(456, p, null, FLAG_ONEWAY)).isTrue();
-    assertThat(transactionsHandled).isEqualTo(2);
-    assertThat(lastCode).isEqualTo(456);
-    assertThat(lastParcel).isSameInstanceAs(p);
+    assertThat(handler.transactionsHandled).isEqualTo(2);
+    assertThat(handler.lastCode).isEqualTo(456);
+    assertThat(handler.lastParcel).isSameInstanceAs(p);
     p.recycle();
   }
 

binder/src/main/java/io/grpc/binder/internal/BinderServer.java

@@ -16,11 +16,14 @@
 
 package io.grpc.binder.internal;
 
+import static android.os.IBinder.FLAG_ONEWAY;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static io.grpc.binder.internal.BinderTransport.SHUTDOWN_TRANSPORT;
 
 import android.os.Binder;
 import android.os.IBinder;
 import android.os.Parcel;
+import android.os.RemoteException;
 import com.google.common.collect.ImmutableList;
 import io.grpc.Attributes;
 import io.grpc.Grpc;
@@ -41,6 +44,8 @@
 import java.net.SocketAddress;
 import java.util.List;
 import java.util.concurrent.ScheduledExecutorService;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.GuardedBy;
 import javax.annotation.concurrent.ThreadSafe;
@@ -55,6 +60,7 @@
  */
 @ThreadSafe
 public final class BinderServer implements InternalServer, LeakSafeOneWayBinder.TransactionHandler {
+  private static final Logger logger = Logger.getLogger(BinderServer.class.getName());
 
   private final ObjectPool<ScheduledExecutorService> executorServicePool;
   private final ImmutableList<ServerStreamTracer.Factory> streamTracerFactories;
@@ -121,7 +127,7 @@ public synchronized void shutdown() {
     if (!shutdown) {
       shutdown = true;
       // Break the connection to the binder. We'll receive no more transactions.
-      hostServiceBinder.detach();
+      hostServiceBinder.setHandler(GoAwayHandler.INSTANCE);
       listener.serverShutdown();
       executorService = executorServicePool.returnObject(executorService);
       transportSecurityShutdownListener.onServerShutdown();
@@ -136,6 +142,12 @@ public String toString() {
   @Override
   public synchronized boolean handleTransaction(int code, Parcel parcel) {
     if (code == BinderTransport.SETUP_TRANSPORT) {
+      if (shutdown) {
+        // An incoming SETUP_TRANSPORT transaction may have already been in-flight when we removed
+        // ourself as TransactionHandler in #shutdown(). So we must check for shutdown again here.
+        return GoAwayHandler.INSTANCE.handleTransaction(code, parcel);
+      }
+
       int version = parcel.readInt();
       // If the client-provided version is more recent, we accept the connection,
       // but specify the older version which we support.
@@ -165,6 +177,28 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) {
     return false;
   }
 
+  static final class GoAwayHandler implements LeakSafeOneWayBinder.TransactionHandler {
+    static final GoAwayHandler INSTANCE = new GoAwayHandler();
+
+    @Override
+    public boolean handleTransaction(int code, Parcel parcel) {
+      if (code == BinderTransport.SETUP_TRANSPORT) {
+        int version = parcel.readInt();
+        if (version >= BinderTransport.EARLIEST_SUPPORTED_WIRE_FORMAT_VERSION) {
+          IBinder callbackBinder = parcel.readStrongBinder();
+          try (ParcelHolder goAwayReply = ParcelHolder.obtain()) {
+            // Send empty flags to avoid a memory leak linked to empty parcels (b/207778694).
+            goAwayReply.get().writeInt(0);
+            callbackBinder.transact(SHUTDOWN_TRANSPORT, goAwayReply.get(), null, FLAG_ONEWAY);
+          } catch (RemoteException re) {
+            logger.log(Level.WARNING, "Couldn't reply to post-shutdown() SETUP_TRANSPORT.", re);
+          }
+        }
+      }
+      return false;
+    }
+  }
+
   /** Fluent builder of {@link BinderServer} instances. */
   public static class Builder {
     @Nullable AndroidComponentAddress listenAddress;

binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java

@@ -68,7 +68,14 @@ public LeakSafeOneWayBinder(TransactionHandler handler) {
   }
 
   public void detach() {
-    handler = null;
+    setHandler(null);
+  }
+
+  /**
+   * Replaces the current {@link TransactionHandler} with `handler`.
+   */
+  public void setHandler(@Nullable TransactionHandler handler) {
+    this.handler = handler;
   }
 
   @Override