binder: synchronize in-use stream updates #12458
Conversation
|
I’m considering adding a unit test for updateInUseStreamsIfNeed() (with VisibleForTesting annotation) |
| @GuardedBy("this") | ||
| void notifyTerminated() { | ||
| if (numInUseStreams.getAndSet(0) > 0) { | ||
| if(numInUseStreams > 0) { |
There was a problem hiding this comment.
Please format this code according to the style guide: https://google.github.io/styleguide/javaguide.html I recommend the https://github.com/google/google-java-format tool
| if (inbound.countsForInUse() && numInUseStreams.decrementAndGet() == 0) { | ||
| clientTransportListener.transportInUse(false); | ||
| } | ||
| updateInUseStreamsIfNeed(inbound.countsForInUse(), -1); |
There was a problem hiding this comment.
Please see "A note on synchronization" at the top of BinderTransport.java. Note that unregisterInbound() is called from Inbound.java's synchronized closeAbnormal() and deliverSuffix() methods (via unregister()) Is your proposed lock acquisition order safe or does it risk deadlock?
There was a problem hiding this comment.
Hello @jdcormie
I’ve updated the PR to address the potential deadlock you mentioned in “A note on synchronization” in BinderTransport.java.
I think the fix ensures listener notifications are executed outside the transport lock via the scheduled executor to maintain safe lock ordering.
I also updated the PR title and description to better reflect the changes, and formatted the code using google-java-format.
Could you please take a look when you have a moment? I’d really appreciate your review and feedback.
becomeStar
changed the title
| // without taking the transport lock to avoid potential deadlocks. | ||
| synchronized (listenerNotifyLock) { | ||
| if (listenerInUse.get() == inUse) { | ||
| clientTransportListener.transportInUse(inUse); |
There was a problem hiding this comment.
Your new listenerNotifyLock may ensure two threads don't call transportInUse() at the same time, but why is that sufficient? According to the Listener API contract, don't we actually need mutual exclusion across all Listener methods?
There was a problem hiding this comment.
Thanks for the earlier feedback.
I updated the PR to ensure mutual exclusion across all Listener callbacks under a single lock and added reconciliation logic for the in-use counter.
Could you please take another look when you have time?
I also rebased the branch to resolve the recent conflicts.
…ronizing 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 grpc#10917
…tside transport lock
- 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
becomeStar
force-pushed
the
binder/sync-inuse-streams
branch
from
a3ea674 to
7f08056
Compare
2 participants
Fixes a race between
newStream()andunregisterInbound()that could cause inconsistenttransportInUse()notifications.Fixes #10917