Skip to content

Commit f084896

Browse files
committed
Stop leaking 'this' from the BinderServerTransport ctor.
Accomplish this by splitting out the linkToDeath() part of setOutgoingBinder(). We also add a test for the DOA case and document why ignoring errors from linkToDeath() is ok.
1 parent 8275580 commit f084896

File tree

4 files changed

+42
-14
lines changed

4 files changed

+42
-14
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,8 @@ protected void handleSetupTransport(Parcel parcel) {
335335
return;
336336
}
337337

338-
if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) {
338+
setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor));
339+
if (!linkToDeathOfOutgoingBinder()) {
339340
shutdownInternal(
340341
Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true);
341342
return;

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,15 @@ public BinderServerTransport(
6969
*/
7070
public synchronized void start(ServerTransportListener serverTransportListener) {
7171
this.listenerPromise.set(serverTransportListener);
72-
if (isShutdown()) {
73-
// It's unlikely, but we could be shutdown externally between construction and start(). One
74-
// possible cause is an extremely short handshake timeout.
75-
return;
76-
}
72+
73+
// Ignore errors. If outgoingBinder is DOA, sendSetupTransaction() will fail with the same code.
74+
linkToDeathOfOutgoingBinder();
7775

7876
sendSetupTransaction();
7977

80-
// Check we're not shutdown again, since a failure inside sendSetupTransaction (or a callback
81-
// it triggers), could have shut us down.
8278
if (isShutdown()) {
79+
// It's unlikely, but we could already be shutdown at this point. One possible cause is an
80+
// extremely short handshake timeout. Another is if the client's binder was dead on arrival.
8381
return;
8482
}
8583

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

Lines changed: 23 additions & 4 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,12 +278,30 @@ 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.
285+
*/
286+
@GuardedBy("this")
287+
protected void setOutgoingBinder(OneWayBinderProxy binder) {
288+
this.outgoingBinder = binderDecorator.decorate(binder);
289+
}
290+
291+
/**
292+
* Registers for notification of the death of the process hosting our peer.
293+
*
294+
* <p>Subclasses should call this ASAP after #setOutgoingBinder but not from a constructor.
295+
*
296+
* <p>Returns true for success and false if the outgoing binder is already dead. Callers should
297+
* shutdown the transport in this case.
298+
*/
280299
@GuardedBy("this")
281-
protected boolean setOutgoingBinder(OneWayBinderProxy binder) {
282-
binder = binderDecorator.decorate(binder);
283-
this.outgoingBinder = binder;
300+
protected boolean linkToDeathOfOutgoingBinder() {
284301
try {
285-
binder.getDelegate().linkToDeath(this, 0);
302+
checkNotNull(outgoingBinder, "Call setOutgoingBinder() first!")
303+
.getDelegate()
304+
.linkToDeath(this, 0);
286305
return true;
287306
} catch (RemoteException re) {
288307
return false;

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

Lines changed: 12 additions & 2 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;
@@ -33,6 +31,7 @@
3331
import io.grpc.Attributes;
3432
import io.grpc.ServerStreamTracer;
3533
import io.grpc.Status;
34+
import io.grpc.binder.FakeDeadBinder;
3635
import io.grpc.internal.FixedObjectPool;
3736
import io.grpc.internal.MockServerTransportListener;
3837
import io.grpc.internal.ObjectPool;
@@ -96,6 +95,17 @@ public void testSetupTransactionFailureReportsMultipleTerminations_b153460678()
9695
assertThat(transportListener.isTerminated()).isTrue();
9796
}
9897

98+
@Test
99+
public void testClientBinderIsDeadOnArrival() throws Exception {
100+
transport = newBinderServerTransportBuilder()
101+
.setCallbackBinder(new FakeDeadBinder())
102+
.build();
103+
transport.start(transportListener);
104+
shadowOf(Looper.getMainLooper()).idle();
105+
106+
assertThat(transportListener.isTerminated()).isTrue();
107+
}
108+
99109
@Test
100110
public void testStartAfterShutdownAndIdle() throws Exception {
101111
transport = newBinderServerTransportBuilder().build();

0 commit comments

Comments
 (0)