Skip to content

Commit 89d77e0

Browse files
authored
Stop leaking this from BinderServerTransport's ctor (#12453)
Accomplish this with a new factory method. Document why ignoring errors from setOutgoingBinder() is ok. Add a test for a dead-on-arrival client binder.
1 parent 9e58ea8 commit 89d77e0

File tree

4 files changed

+40
-8
lines changed

4 files changed

+40
-8
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) {
179179
checkNotNull(executor, "Not started?"));
180180
// Create a new transport and let our listener know about it.
181181
BinderServerTransport transport =
182-
new BinderServerTransport(
182+
BinderServerTransport.create(
183183
executorServicePool,
184184
attrsBuilder.build(),
185185
streamTracerFactories,

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

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,21 +42,35 @@ public final class BinderServerTransport extends BinderTransport implements Serv
4242
@GuardedBy("this")
4343
private final SimplePromise<ServerTransportListener> listenerPromise = new SimplePromise<>();
4444

45+
private BinderServerTransport(
46+
ObjectPool<ScheduledExecutorService> executorServicePool,
47+
Attributes attributes,
48+
List<ServerStreamTracer.Factory> streamTracerFactories,
49+
OneWayBinderProxy.Decorator binderDecorator) {
50+
super(executorServicePool, attributes, binderDecorator, buildLogId(attributes));
51+
this.streamTracerFactories = streamTracerFactories;
52+
}
53+
4554
/**
4655
* Constructs a new transport instance.
4756
*
4857
* @param binderDecorator used to decorate 'callbackBinder', for fault injection.
4958
*/
50-
public BinderServerTransport(
59+
public static BinderServerTransport create(
5160
ObjectPool<ScheduledExecutorService> executorServicePool,
5261
Attributes attributes,
5362
List<ServerStreamTracer.Factory> streamTracerFactories,
5463
OneWayBinderProxy.Decorator binderDecorator,
5564
IBinder callbackBinder) {
56-
super(executorServicePool, attributes, binderDecorator, buildLogId(attributes));
57-
this.streamTracerFactories = streamTracerFactories;
65+
BinderServerTransport transport =
66+
new BinderServerTransport(
67+
executorServicePool, attributes, streamTracerFactories, binderDecorator);
5868
// TODO(jdcormie): Plumb in the Server's executor() and use it here instead.
59-
setOutgoingBinder(OneWayBinderProxy.wrap(callbackBinder, getScheduledExecutorService()));
69+
// No need to handle failure here because if 'callbackBinder' is already dead, we'll notice it
70+
// again in start() when we send the first transaction.
71+
transport.setOutgoingBinder(
72+
OneWayBinderProxy.wrap(callbackBinder, transport.getScheduledExecutorService()));
73+
return transport;
6074
}
6175

6276
/**

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ protected enum TransportState {
159159
private final ObjectPool<ScheduledExecutorService> executorServicePool;
160160
private final ScheduledExecutorService scheduledExecutorService;
161161
private final InternalLogId logId;
162+
162163
@GuardedBy("this")
163164
private final LeakSafeOneWayBinder incomingBinder;
164165

@@ -277,6 +278,14 @@ final void setState(TransportState newState) {
277278
transportState = newState;
278279
}
279280

281+
/**
282+
* Sets the binder to use for sending subsequent transactions to our peer.
283+
*
284+
* <p>Subclasses should call this as early as possible but not from a constructor.
285+
*
286+
* <p>Returns true for success, false if the process hosting 'binder' is already dead. Callers are
287+
* responsible for handling this.
288+
*/
280289
@GuardedBy("this")
281290
protected boolean setOutgoingBinder(OneWayBinderProxy binder) {
282291
binder = binderDecorator.decorate(binder);

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,8 @@
2121
import static org.mockito.ArgumentMatchers.anyInt;
2222
import static org.mockito.ArgumentMatchers.isNull;
2323
import static org.mockito.Mockito.doThrow;
24-
import static org.mockito.Mockito.when;
2524
import static org.robolectric.Shadows.shadowOf;
2625

27-
import android.os.DeadObjectException;
2826
import android.os.IBinder;
2927
import android.os.Looper;
3028
import android.os.Parcel;
@@ -96,6 +94,17 @@ public void testSetupTransactionFailureReportsMultipleTerminations_b153460678()
9694
assertThat(transportListener.isTerminated()).isTrue();
9795
}
9896

97+
@Test
98+
public void testClientBinderIsDeadOnArrival() throws Exception {
99+
transport = newBinderServerTransportBuilder()
100+
.setCallbackBinder(new FakeDeadBinder())
101+
.build();
102+
transport.start(transportListener);
103+
shadowOf(Looper.getMainLooper()).idle();
104+
105+
assertThat(transportListener.isTerminated()).isTrue();
106+
}
107+
99108
@Test
100109
public void testStartAfterShutdownAndIdle() throws Exception {
101110
transport = newBinderServerTransportBuilder().build();
@@ -125,7 +134,7 @@ static class BinderServerTransportBuilder {
125134
IBinder callbackBinder;
126135

127136
public BinderServerTransport build() {
128-
return new BinderServerTransport(
137+
return BinderServerTransport.create(
129138
executorServicePool, attributes, streamTracerFactories, binderDecorator, callbackBinder);
130139
}
131140

0 commit comments

Comments
 (0)