Skip to content

binder: Rationalize @ThreadSafe-ty of BinderTransport. #12130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 6, 2025
Merged

binder: Rationalize @ThreadSafe-ty of BinderTransport. #12130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c93bcc
binder: Rationalize @ThreadSafe-ty of BinderTransport.
jdcormie Jun 5, 2025
5703688
Annotate LSOWB
jdcormie Jun 5, 2025
cc05933
Undo synchronization change just to keep the PR smaller
jdcormie Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 15 additions & 13 deletions binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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(
Expand All @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -483,11 +486,12 @@ 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) {
numIncomingBytes += size;
if ((numIncomingBytes - acknowledgedIncomingBytes) > TRANSACTION_BYTES_WINDOW_FORCE_ACK) {
synchronized (this) {
sendAcknowledgeBytes(checkNotNull(outgoingBinder));
sendAcknowledgeBytes(checkNotNull(outgoingBinder), numIncomingBytes);
}
acknowledgedIncomingBytes = numIncomingBytes;
}
return true;
}
Expand Down Expand Up @@ -519,10 +523,8 @@ private final void handlePing(Parcel requestParcel) {
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);
Expand Down
2 changes: 2 additions & 0 deletions binder/src/main/java/io/grpc/binder/internal/LeakSafeOneWayBinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down