From 66ef69195ccdaa6ce75de828524bf614ec7863c5 Mon Sep 17 00:00:00 2001 From: becomeStar Date: Fri, 31 Oct 2025 13:24:05 +0900 Subject: [PATCH 1/3] binder: fix race between newStream() and unregisterInbound() by synchronizing in-use updates Previously, concurrent calls to newStream() and unregisterInbound() could both update numInUseStreams and invoke transportInUse() in conflicting order, leading to inconsistent listener state. This change synchronizes updates and only notifies the listener on transitions between 0 and >0. Fixes #10917 --- .../internal/BinderClientTransport.java | 44 ++++++++++++++----- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index 00437956a51..e422d486f50 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -59,7 +59,7 @@ import io.grpc.internal.StatsTraceContext; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.atomic.AtomicInteger; + import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; @@ -78,7 +78,12 @@ public final class BinderClientTransport extends BinderTransport private final ClientHandshake handshake; /** Number of ongoing calls which keep this transport "in-use". */ - private final AtomicInteger numInUseStreams; + @GuardedBy("this") + private int numInUseStreams; + + /** Last in-use state that was reported to the listener */ + @GuardedBy("this") + private boolean listenerInUse; private final long readyTimeoutMillis; private final PingTracker pingTracker; @@ -119,9 +124,11 @@ public BinderClientTransport( Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE); this.preAuthorizeServer = preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers; + this.handshake = factory.useLegacyAuthStrategy ? new LegacyClientHandshake() : new V2ClientHandshake(); - numInUseStreams = new AtomicInteger(); + numInUseStreams = 0; + listenerInUse = false; pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); serviceBinding = new ServiceBinding( @@ -265,9 +272,7 @@ public synchronized ClientStream newStream( return newFailingClientStream(failure, attributes, headers, tracers); } - if (inbound.countsForInUse() && numInUseStreams.getAndIncrement() == 0) { - clientTransportListener.transportInUse(true); - } + updateInUseStreamsIfNeed(inbound.countsForInUse(), 1); Outbound.ClientOutbound outbound = new Outbound.ClientOutbound(this, callId, method, headers, statsTraceContext); if (method.getType().clientSendsOneMessage()) { @@ -279,9 +284,7 @@ public synchronized ClientStream newStream( @Override protected void unregisterInbound(Inbound inbound) { - if (inbound.countsForInUse() && numInUseStreams.decrementAndGet() == 0) { - clientTransportListener.transportInUse(false); - } + updateInUseStreamsIfNeed(inbound.countsForInUse(), -1); super.unregisterInbound(inbound); } @@ -311,7 +314,9 @@ void notifyShutdown(Status status) { @Override @GuardedBy("this") void notifyTerminated() { - if (numInUseStreams.getAndSet(0) > 0) { + if(numInUseStreams > 0) { + numInUseStreams = 0; + listenerInUse = false; clientTransportListener.transportInUse(false); } if (readyTimeoutFuture != null) { @@ -452,6 +457,25 @@ private synchronized void handleAuthResult(Throwable t) { Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); } + /** Updates in-use-stream count and notifies listener only on transitions between 0 and >0 */ + private synchronized void updateInUseStreamsIfNeed(boolean countsForInUse, int delta) { + if(!countsForInUse) { + return; + } + + numInUseStreams += delta; + if(numInUseStreams < 0) { + // Defensive: prevent negative due to unexpected double-decrement + numInUseStreams = 0; + } + + boolean nowInUseStream = numInUseStreams > 0; + if(nowInUseStream != listenerInUse) { + listenerInUse = nowInUseStream; + clientTransportListener.transportInUse(nowInUseStream); + } + } + @GuardedBy("this") @Override protected void handlePingResponse(Parcel parcel) { From 896c2fd9d714b568327cd0e7ef67658c04fc9af2 Mon Sep 17 00:00:00 2001 From: becomeStar Date: Thu, 6 Nov 2025 00:27:53 +0900 Subject: [PATCH 2/3] fix: avoid potential deadlock by scheduling listener notifications outside transport lock --- .../internal/BinderClientTransport.java | 89 ++++++++++++++----- 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index e422d486f50..bf10728839f 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -25,8 +25,11 @@ import android.os.IBinder; import android.os.Parcel; import android.os.Process; + import androidx.annotation.BinderThread; import androidx.annotation.MainThread; + +import com.google.common.base.Preconditions; import com.google.common.base.Ticker; import com.google.common.util.concurrent.FutureCallback; import com.google.common.util.concurrent.Futures; @@ -59,6 +62,8 @@ import io.grpc.internal.StatsTraceContext; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; @@ -78,12 +83,13 @@ public final class BinderClientTransport extends BinderTransport private final ClientHandshake handshake; /** Number of ongoing calls which keep this transport "in-use". */ - @GuardedBy("this") - private int numInUseStreams; + private final AtomicInteger numInUseStreams; /** Last in-use state that was reported to the listener */ - @GuardedBy("this") - private boolean listenerInUse; + private final AtomicBoolean listenerInUse; + + /** Synchronizes transport listener callbacks */ + private final Object listenerNotifyLock; private final long readyTimeoutMillis; private final PingTracker pingTracker; @@ -124,11 +130,11 @@ public BinderClientTransport( Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE); this.preAuthorizeServer = preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers; - this.handshake = factory.useLegacyAuthStrategy ? new LegacyClientHandshake() : new V2ClientHandshake(); - numInUseStreams = 0; - listenerInUse = false; + this.numInUseStreams = new AtomicInteger(); + this.listenerInUse = new AtomicBoolean(); + this.listenerNotifyLock = new Object(); pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id)); serviceBinding = new ServiceBinding( @@ -272,7 +278,7 @@ public synchronized ClientStream newStream( return newFailingClientStream(failure, attributes, headers, tracers); } - updateInUseStreamsIfNeed(inbound.countsForInUse(), 1); + updateInUseStreamsCountIfNeeded(inbound.countsForInUse(), 1); Outbound.ClientOutbound outbound = new Outbound.ClientOutbound(this, callId, method, headers, statsTraceContext); if (method.getType().clientSendsOneMessage()) { @@ -284,7 +290,7 @@ public synchronized ClientStream newStream( @Override protected void unregisterInbound(Inbound inbound) { - updateInUseStreamsIfNeed(inbound.countsForInUse(), -1); + updateInUseStreamsCountIfNeeded(inbound.countsForInUse(), -1); super.unregisterInbound(inbound); } @@ -314,9 +320,8 @@ void notifyShutdown(Status status) { @Override @GuardedBy("this") void notifyTerminated() { - if(numInUseStreams > 0) { - numInUseStreams = 0; - listenerInUse = false; + if (numInUseStreams.getAndSet(0) > 0) { + listenerInUse.set(false); clientTransportListener.transportInUse(false); } if (readyTimeoutFuture != null) { @@ -457,25 +462,63 @@ private synchronized void handleAuthResult(Throwable t) { Status.INTERNAL.withDescription("Could not evaluate SecurityPolicy").withCause(t), true); } - /** Updates in-use-stream count and notifies listener only on transitions between 0 and >0 */ - private synchronized void updateInUseStreamsIfNeed(boolean countsForInUse, int delta) { - if(!countsForInUse) { + /** + * Updates in-use stream count and notifies listener only on transitions between 0 and >0, without + * acquiring the transport lock. + */ + private void updateInUseStreamsCountIfNeeded(boolean countsForInUse, int delta) { + Preconditions.checkArgument(delta == -1 || delta == 1, "stream count delta must be -1 or +1"); + if (!countsForInUse) { return; } + int prev, next; - numInUseStreams += delta; - if(numInUseStreams < 0) { - // Defensive: prevent negative due to unexpected double-decrement - numInUseStreams = 0; + if (delta > 0) { + next = numInUseStreams.incrementAndGet(); + prev = next - 1; + } else { + prev = numInUseStreams.get(); + int updated; + + while (true) { + int current = prev; + int newValue = current > 0 ? current - 1 : 0; + if (numInUseStreams.compareAndSet(current, newValue)) { + updated = newValue; + break; + } + prev = numInUseStreams.get(); + } + next = updated; } - boolean nowInUseStream = numInUseStreams > 0; - if(nowInUseStream != listenerInUse) { - listenerInUse = nowInUseStream; - clientTransportListener.transportInUse(nowInUseStream); + boolean prevInUse = prev > 0; + boolean nextInUse = next > 0; + + if (prevInUse != nextInUse) { + if (listenerInUse.compareAndSet(prevInUse, nextInUse)) { + scheduleTransportInUseNotification(nextInUse); + } } } + private void scheduleTransportInUseNotification(final boolean inUse) { + getScheduledExecutorService() + .execute( + new Runnable() { + @Override + public void run() { + // Provide external synchronization as required by Listener contract, + // without taking the transport lock to avoid potential deadlocks. + synchronized (listenerNotifyLock) { + if (listenerInUse.get() == inUse) { + clientTransportListener.transportInUse(inUse); + } + } + } + }); + } + @GuardedBy("this") @Override protected void handlePingResponse(Parcel parcel) { From 7f080567d8b6237ffebb175369164cbd92a508a2 Mon Sep 17 00:00:00 2001 From: becomeStar Date: Mon, 17 Nov 2025 23:37:28 +0900 Subject: [PATCH 3/3] binder: make Listener callbacks mutually exclusive and fix in-use race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Serialize transportShutdown/transportTerminated/transportReady/transportInUse under listener lock - Atomic in-use counter with reconcile to prevent incorrect false when −1/+1 race occurs - Dispatch callbacks asynchronously to avoid lock-order deadlocks - Behavior unchanged for users; improves correctness under concurrency --- .../internal/BinderClientTransport.java | 94 +++++++++++++++---- 1 file changed, 75 insertions(+), 19 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java index bf10728839f..afe368085a6 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java @@ -314,22 +314,38 @@ public synchronized void shutdownNow(Status reason) { @Override @GuardedBy("this") void notifyShutdown(Status status) { - clientTransportListener.transportShutdown(status); + // Defer to listener executor with external synchronization + scheduleOnListener( + new Runnable() { + @Override + public void run() { + clientTransportListener.transportShutdown(status); + } + }); } @Override @GuardedBy("this") void notifyTerminated() { if (numInUseStreams.getAndSet(0) > 0) { - listenerInUse.set(false); - clientTransportListener.transportInUse(false); + if (listenerInUse.compareAndSet(true, false)) { + scheduleTransportInUseNotification(false); + } else { + listenerInUse.set(false); + } } if (readyTimeoutFuture != null) { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; } serviceBinding.unbind(); - clientTransportListener.transportTerminated(); + scheduleOnListener( + new Runnable() { + @Override + public void run() { + clientTransportListener.transportTerminated(); + } + }); } @Override @@ -449,8 +465,11 @@ public void handleSetupTransport() { @GuardedBy("this") private void onHandshakeComplete() { setState(TransportState.READY); - attributes = clientTransportListener.filterTransport(attributes); - clientTransportListener.transportReady(); + final Attributes currentAttrs = attributes; + // Perform filter on listener thread with external synchronization, then update attrs and + // notify ready without holding transport lock to avoid deadlocks. + scheduleFilterTransportAndReady(currentAttrs); + if (readyTimeoutFuture != null) { readyTimeoutFuture.cancel(false); readyTimeoutFuture = null; @@ -471,34 +490,32 @@ private void updateInUseStreamsCountIfNeeded(boolean countsForInUse, int delta) if (!countsForInUse) { return; } - int prev, next; if (delta > 0) { - next = numInUseStreams.incrementAndGet(); - prev = next - 1; + numInUseStreams.incrementAndGet(); } else { - prev = numInUseStreams.get(); - int updated; + // Decrement with floor at 0 + int prev = numInUseStreams.get(); while (true) { int current = prev; int newValue = current > 0 ? current - 1 : 0; if (numInUseStreams.compareAndSet(current, newValue)) { - updated = newValue; break; } prev = numInUseStreams.get(); } - next = updated; } + reconcileInUseState(); + } - boolean prevInUse = prev > 0; - boolean nextInUse = next > 0; + /** Reconcile listenerInUse with the current stream count to avoid stale toggles under races. */ + private void reconcileInUseState() { + boolean nowInUse = numInUseStreams.get() > 0; + boolean prev = listenerInUse.get(); - if (prevInUse != nextInUse) { - if (listenerInUse.compareAndSet(prevInUse, nextInUse)) { - scheduleTransportInUseNotification(nextInUse); - } + if(prev != nowInUse && listenerInUse.compareAndSet(prev, nowInUse)) { + scheduleTransportInUseNotification(nowInUse); } } @@ -519,6 +536,45 @@ public void run() { }); } + private void scheduleFilterTransportAndReady(final Attributes attrsSnapshot) { + getScheduledExecutorService() + .execute( + new Runnable() { + @Override + public void run() { + final Attributes filtered; + synchronized (listenerNotifyLock) { + filtered = clientTransportListener.filterTransport(attrsSnapshot); + } + + synchronized (BinderClientTransport.class) { + attributes = filtered; + } + + scheduleOnListener( + new Runnable() { + @Override + public void run() { + clientTransportListener.transportReady(); + } + }); + } + }); + } + + private void scheduleOnListener(final Runnable task) { + getScheduledExecutorService() + .execute( + new Runnable() { + @Override + public void run() { + synchronized (listenerNotifyLock) { + task.run(); + } + } + }); + } + @GuardedBy("this") @Override protected void handlePingResponse(Parcel parcel) {