From 1c93bcca32f889310911870651308349a484cbfb Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 4 Jun 2025 17:30:08 -0700 Subject: [PATCH 1/3] binder: Rationalize @ThreadSafe-ty of BinderTransport. - Use @BinderThread to document restrictions on methods and certain fields. - Make TransactionHandler non-public since only Android should call it. - Replace an unnecessary AtomicLong with a plain old long. --- .../grpc/binder/internal/BinderTransport.java | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index f61c455edd5..022c23f91bb 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -29,6 +29,7 @@ import android.os.Process; import android.os.RemoteException; import android.os.TransactionTooLargeException; +import androidx.annotation.BinderThread; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Ticker; import com.google.common.base.Verify; @@ -105,8 +106,7 @@ * https://github.com/grpc/proposal/blob/master/L73-java-binderchannel/wireformat.md */ @ThreadSafe -public abstract class BinderTransport - implements LeakSafeOneWayBinder.TransactionHandler, IBinder.DeathRecipient { +public abstract class BinderTransport implements IBinder.DeathRecipient { private static final Logger logger = Logger.getLogger(BinderTransport.class.getName()); @@ -210,9 +210,11 @@ protected enum TransportState { private final FlowController flowController; /** The number of incoming bytes we've received. */ - private final AtomicLong numIncomingBytes; + // Only read/written on @BinderThread. + private long numIncomingBytes; /** The number of incoming bytes we've told our peer we've received. */ + // Only read/written on @BinderThread. private long acknowledgedIncomingBytes; private BinderTransport( @@ -225,10 +227,9 @@ private BinderTransport( this.attributes = attributes; this.logId = logId; scheduledExecutorService = executorServicePool.getObject(); - incomingBinder = new LeakSafeOneWayBinder(this); + incomingBinder = new LeakSafeOneWayBinder(this::handleTransaction); ongoingCalls = new ConcurrentHashMap<>(); flowController = new FlowController(TRANSACTION_BYTES_WINDOW); - numIncomingBytes = new AtomicLong(); } // Override in child class. @@ -423,8 +424,9 @@ final void sendOutOfBandClose(int callId, Status status) { } } - @Override - public final boolean handleTransaction(int code, Parcel parcel) { + @BinderThread + @VisibleForTesting + final boolean handleTransaction(int code, Parcel parcel) { try { return handleTransactionInternal(code, parcel); } catch (RuntimeException e) { @@ -440,6 +442,7 @@ public final boolean handleTransaction(int code, Parcel parcel) { } } + @BinderThread private boolean handleTransactionInternal(int code, Parcel parcel) { if (code < FIRST_CALL_ID) { synchronized (this) { @@ -483,11 +486,10 @@ private boolean handleTransactionInternal(int code, Parcel parcel) { if (inbound != null) { inbound.handleTransaction(parcel); } - long nib = numIncomingBytes.addAndGet(size); - if ((nib - acknowledgedIncomingBytes) > TRANSACTION_BYTES_WINDOW_FORCE_ACK) { - synchronized (this) { - sendAcknowledgeBytes(checkNotNull(outgoingBinder)); - } + numIncomingBytes += size; + if ((numIncomingBytes - acknowledgedIncomingBytes) > TRANSACTION_BYTES_WINDOW_FORCE_ACK) { + sendAcknowledgeBytes(checkNotNull(outgoingBinder), numIncomingBytes); + acknowledgedIncomingBytes = numIncomingBytes; } return true; } @@ -518,16 +520,15 @@ private final void handlePing(Parcel requestParcel) { @GuardedBy("this") protected void handlePingResponse(Parcel parcel) {} - @GuardedBy("this") - private void sendAcknowledgeBytes(OneWayBinderProxy iBinder) { + private void sendAcknowledgeBytes(OneWayBinderProxy iBinder, long n) { // Send a transaction to acknowledge reception of incoming data. - long n = numIncomingBytes.get(); - acknowledgedIncomingBytes = n; try (ParcelHolder parcel = ParcelHolder.obtain()) { parcel.get().writeLong(n); iBinder.transact(ACKNOWLEDGE_BYTES, parcel); } catch (RemoteException re) { - shutdownInternal(statusFromRemoteException(re), true); + synchronized (this) { + shutdownInternal(statusFromRemoteException(re), true); + } } } From 57036881cac82cc3bea84f25371bbc8b7acfd373 Mon Sep 17 00:00:00 2001 From: John Cormie Date: Wed, 4 Jun 2025 17:38:35 -0700 Subject: [PATCH 2/3] Annotate LSOWB --- .../main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java b/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java index a12a5cb13cc..e7837b520f8 100644 --- a/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java +++ b/binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java @@ -19,6 +19,7 @@ import android.os.Binder; import android.os.IBinder; import android.os.Parcel; +import androidx.annotation.BinderThread; import io.grpc.Internal; import java.util.logging.Level; import java.util.logging.Logger; @@ -58,6 +59,7 @@ public interface TransactionHandler { * @return the value to return from {@link Binder#onTransact}. NB: "oneway" semantics mean this * result will not delivered to the caller of {@link IBinder#transact} */ + @BinderThread boolean handleTransaction(int code, Parcel data); } From cc05933883f1e7d27bceaa22949b6bdc5864480d Mon Sep 17 00:00:00 2001 From: John Cormie Date: Thu, 5 Jun 2025 09:24:47 -0700 Subject: [PATCH 3/3] Undo synchronization change just to keep the PR smaller --- .../java/io/grpc/binder/internal/BinderTransport.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java index 022c23f91bb..c8900031432 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransport.java @@ -488,7 +488,9 @@ private boolean handleTransactionInternal(int code, Parcel parcel) { } numIncomingBytes += size; if ((numIncomingBytes - acknowledgedIncomingBytes) > TRANSACTION_BYTES_WINDOW_FORCE_ACK) { - sendAcknowledgeBytes(checkNotNull(outgoingBinder), numIncomingBytes); + synchronized (this) { + sendAcknowledgeBytes(checkNotNull(outgoingBinder), numIncomingBytes); + } acknowledgedIncomingBytes = numIncomingBytes; } return true; @@ -520,15 +522,14 @@ private final void handlePing(Parcel requestParcel) { @GuardedBy("this") protected void handlePingResponse(Parcel parcel) {} + @GuardedBy("this") private void sendAcknowledgeBytes(OneWayBinderProxy iBinder, long n) { // Send a transaction to acknowledge reception of incoming data. try (ParcelHolder parcel = ParcelHolder.obtain()) { parcel.get().writeLong(n); iBinder.transact(ACKNOWLEDGE_BYTES, parcel); } catch (RemoteException re) { - synchronized (this) { - shutdownInternal(statusFromRemoteException(re), true); - } + shutdownInternal(statusFromRemoteException(re), true); } }