binder: synchronize in-use stream updates #12458
Conversation
…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
|
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.
…tside transport lock
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?
2 participants
Fixes a race between
newStream()andunregisterInbound()that could cause inconsistenttransportInUse()notifications.Fixes #10917