Stop leaking this from BinderServerTransport's ctor

Also add a test for the DOA case and document why ignoring errors from setOutgoingBinder() is ok.

Author: jdcormie

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

@@ -179,7 +179,7 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) {
               checkNotNull(executor, "Not started?"));
           // Create a new transport and let our listener know about it.
           BinderServerTransport transport =
-              new BinderServerTransport(
+              BinderServerTransport.create(
                   executorServicePool,
                   attrsBuilder.build(),
                   streamTracerFactories,

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

@@ -42,21 +42,35 @@ public final class BinderServerTransport extends BinderTransport implements Serv
   @GuardedBy("this")
   private final SimplePromise<ServerTransportListener> listenerPromise = new SimplePromise<>();
 
+  private BinderServerTransport(
+      ObjectPool<ScheduledExecutorService> executorServicePool,
+      Attributes attributes,
+      List<ServerStreamTracer.Factory> streamTracerFactories,
+      OneWayBinderProxy.Decorator binderDecorator) {
+    super(executorServicePool, attributes, binderDecorator, buildLogId(attributes));
+    this.streamTracerFactories = streamTracerFactories;
+  }
+
   /**
    * Constructs a new transport instance.
    *
    * @param binderDecorator used to decorate 'callbackBinder', for fault injection.
    */
-  public BinderServerTransport(
+  public static BinderServerTransport create(
       ObjectPool<ScheduledExecutorService> executorServicePool,
       Attributes attributes,
       List<ServerStreamTracer.Factory> streamTracerFactories,
       OneWayBinderProxy.Decorator binderDecorator,
       IBinder callbackBinder) {
-    super(executorServicePool, attributes, binderDecorator, buildLogId(attributes));
-    this.streamTracerFactories = streamTracerFactories;
+    BinderServerTransport transport =
+        new BinderServerTransport(
+            executorServicePool, attributes, streamTracerFactories, binderDecorator);
     // TODO(jdcormie): Plumb in the Server's executor() and use it here instead.
-    setOutgoingBinder(OneWayBinderProxy.wrap(callbackBinder, getScheduledExecutorService()));
+    // No need to handle failure here because if 'callbackBinder' is already dead, we'll notice it
+    // again in start() when we send the first transaction.
+    transport.setOutgoingBinder(
+        OneWayBinderProxy.wrap(callbackBinder, transport.getScheduledExecutorService()));
+    return transport;
   }
 
   /**

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

@@ -159,6 +159,7 @@ protected enum TransportState {
   private final ObjectPool<ScheduledExecutorService> executorServicePool;
   private final ScheduledExecutorService scheduledExecutorService;
   private final InternalLogId logId;
+
   @GuardedBy("this")
   private final LeakSafeOneWayBinder incomingBinder;
 
@@ -277,6 +278,14 @@ final void setState(TransportState newState) {
     transportState = newState;
   }
 
+  /**
+   * Sets the binder to use for sending subsequent transactions to our peer.
+   *
+   * <p>Subclasses should call this as early as possible but not from a constructor.
+   *
+   * <p>Returns true for success, false if the process hosting 'binder' is already dead. Callers are
+   * responsible for handling this.
+   */
   @GuardedBy("this")
   protected boolean setOutgoingBinder(OneWayBinderProxy binder) {
     binder = binderDecorator.decorate(binder);

binder/src/test/java/io/grpc/binder/internal/BinderServerTransportTest.java

@@ -21,10 +21,8 @@
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.doThrow;
-import static org.mockito.Mockito.when;
 import static org.robolectric.Shadows.shadowOf;
 
-import android.os.DeadObjectException;
 import android.os.IBinder;
 import android.os.Looper;
 import android.os.Parcel;
@@ -33,6 +31,7 @@
 import io.grpc.Attributes;
 import io.grpc.ServerStreamTracer;
 import io.grpc.Status;
+import io.grpc.binder.FakeDeadBinder;
 import io.grpc.internal.FixedObjectPool;
 import io.grpc.internal.MockServerTransportListener;
 import io.grpc.internal.ObjectPool;
@@ -96,6 +95,17 @@ public void testSetupTransactionFailureReportsMultipleTerminations_b153460678()
     assertThat(transportListener.isTerminated()).isTrue();
   }
 
+  @Test
+  public void testClientBinderIsDeadOnArrival() throws Exception {
+    transport = newBinderServerTransportBuilder()
+        .setCallbackBinder(new FakeDeadBinder())
+        .build();
+    transport.start(transportListener);
+    shadowOf(Looper.getMainLooper()).idle();
+
+    assertThat(transportListener.isTerminated()).isTrue();
+  }
+
   @Test
   public void testStartAfterShutdownAndIdle() throws Exception {
     transport = newBinderServerTransportBuilder().build();
@@ -125,7 +135,7 @@ static class BinderServerTransportBuilder {
     IBinder callbackBinder;
 
     public BinderServerTransport build() {
-      return new BinderServerTransport(
+      return BinderServerTransport.create(
           executorServicePool, attributes, streamTracerFactories, binderDecorator, callbackBinder);
     }