Skip to content

Commit a3ea674

Browse files
committed
binder: make Listener callbacks mutually exclusive and fix in-use race
- 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
1 parent e32c264 commit a3ea674

File tree

1 file changed

+75
-19
lines changed

1 file changed

+75
-19
lines changed

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

Lines changed: 75 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -308,22 +308,38 @@ public synchronized void shutdownNow(Status reason) {
308308
@Override
309309
@GuardedBy("this")
310310
void notifyShutdown(Status status) {
311-
clientTransportListener.transportShutdown(status);
311+
// Defer to listener executor with external synchronization
312+
scheduleOnListener(
313+
new Runnable() {
314+
@Override
315+
public void run() {
316+
clientTransportListener.transportShutdown(status);
317+
}
318+
});
312319
}
313320

314321
@Override
315322
@GuardedBy("this")
316323
void notifyTerminated() {
317324
if (numInUseStreams.getAndSet(0) > 0) {
318-
listenerInUse.set(false);
319-
clientTransportListener.transportInUse(false);
325+
if (listenerInUse.compareAndSet(true, false)) {
326+
scheduleTransportInUseNotification(false);
327+
} else {
328+
listenerInUse.set(false);
329+
}
320330
}
321331
if (readyTimeoutFuture != null) {
322332
readyTimeoutFuture.cancel(false);
323333
readyTimeoutFuture = null;
324334
}
325335
serviceBinding.unbind();
326-
clientTransportListener.transportTerminated();
336+
scheduleOnListener(
337+
new Runnable() {
338+
@Override
339+
public void run() {
340+
clientTransportListener.transportTerminated();
341+
}
342+
});
327343
}
328344

329345
@Override
@@ -388,8 +404,11 @@ private synchronized void handleAuthResult(Status authorization) {
388404
}
389405

390406
setState(TransportState.READY);
391-
attributes = clientTransportListener.filterTransport(attributes);
392-
clientTransportListener.transportReady();
407+
final Attributes currentAttrs = attributes;
408+
// Perform filter on listener thread with external synchronization, then update attrs and
409+
// notify ready without holding transport lock to avoid deadlocks.
410+
scheduleFilterTransportAndReady(currentAttrs);
411+
393412
if (readyTimeoutFuture != null) {
394413
readyTimeoutFuture.cancel(false);
395414
readyTimeoutFuture = null;
@@ -410,34 +429,32 @@ private void updateInUseStreamsCountIfNeeded(boolean countsForInUse, int delta)
410429
if (!countsForInUse) {
411430
return;
412431
}
413-
int prev, next;
414432

415433
if (delta > 0) {
416-
next = numInUseStreams.incrementAndGet();
417-
prev = next - 1;
434+
numInUseStreams.incrementAndGet();
418435
} else {
419-
prev = numInUseStreams.get();
420-
int updated;
436+
// Decrement with floor at 0
437+
int prev = numInUseStreams.get();
421438

422439
while (true) {
423440
int current = prev;
424441
int newValue = current > 0 ? current - 1 : 0;
425442
if (numInUseStreams.compareAndSet(current, newValue)) {
426-
updated = newValue;
427443
break;
428444
}
429445
prev = numInUseStreams.get();
430446
}
431-
next = updated;
432447
}
448+
reconcileInUseState();
449+
}
433450

434-
boolean prevInUse = prev > 0;
435-
boolean nextInUse = next > 0;
451+
/** Reconcile listenerInUse with the current stream count to avoid stale toggles under races. */
452+
private void reconcileInUseState() {
453+
boolean nowInUse = numInUseStreams.get() > 0;
454+
boolean prev = listenerInUse.get();
436455

437-
if (prevInUse != nextInUse) {
438-
if (listenerInUse.compareAndSet(prevInUse, nextInUse)) {
439-
scheduleTransportInUseNotification(nextInUse);
440-
}
456+
if(prev != nowInUse && listenerInUse.compareAndSet(prev, nowInUse)) {
457+
scheduleTransportInUseNotification(nowInUse);
441458
}
442459
}
443460

@@ -458,6 +475,45 @@ public void run() {
458475
});
459476
}
460477

478+
private void scheduleFilterTransportAndReady(final Attributes attrsSnapshot) {
479+
getScheduledExecutorService()
480+
.execute(
481+
new Runnable() {
482+
@Override
483+
public void run() {
484+
final Attributes filtered;
485+
synchronized (listenerNotifyLock) {
486+
filtered = clientTransportListener.filterTransport(attrsSnapshot);
487+
}
488+
489+
synchronized (BinderClientTransport.class) {
490+
attributes = filtered;
491+
}
492+
493+
scheduleOnListener(
494+
new Runnable() {
495+
@Override
496+
public void run() {
497+
clientTransportListener.transportReady();
498+
}
499+
});
500+
}
501+
});
502+
}
503+
504+
private void scheduleOnListener(final Runnable task) {
505+
getScheduledExecutorService()
506+
.execute(
507+
new Runnable() {
508+
@Override
509+
public void run() {
510+
synchronized (listenerNotifyLock) {
511+
task.run();
512+
}
513+
}
514+
});
515+
}
516+
461517
@GuardedBy("this")
462518
@Override
463519
protected void handlePingResponse(Parcel parcel) {

0 commit comments

Comments
 (0)