Fix a deadlock in TransportSet. #2258
Conversation
zhangkun83
added
the
TODO:backport
| @@ -249,9 +249,14 @@ public void run() { | |||
| delayedTransport.endBackoff(); | |||
| boolean shutdownDelayedTransport = false; | |||
| Runnable runnable = null; | |||
| // TransportSet as a channel layer class should not call into transport methods while | |||
| // holding the lock, thus we call hasPendingStreams() outside of the lock. It will cause | |||
| // a _benign_ race where the TransportSet may transition to CONNECTING when there is not | |||
There was a problem hiding this comment.
I believe the race is also pre-existing, since at any time obtainActiveTransport() can return the delayed transport and a new stream be created.
There was a problem hiding this comment.
That sounds a real issue. If EndOfCurrentBackoff wins in the race with newStream(), the application will see a spurious shutdown error.
There was a problem hiding this comment.
I don't see where the shutdown error would come from because the delayed transport becomes pass-through to the supplier for newStream(), instead of failing new streams after shutdown.
Instead of calling startNewTransport() here, we would instead enter IDLE. But since we call setTransportSupplier with a supplier that calls obtainActiveTransport, then if the race occurs there will just be a call to obtainActiveTransport that will trigger entering CONNECTING.
The only issue is that the stream will wrapped twice with a DelayedStream. But since the race should only happen at most once per stream, we decided that it wasn't a big deal.
There was a problem hiding this comment.
You are right... I lost my touch with delayed transport after not touching it for a while
|
LGTM |
Honor the lock order that transport lock > channel lock. Resolves grpc#2246
zhangkun83
force-pushed
the
transportset_deadlock
branch
from
211e476 to
3d4ae36
Compare
zhangkun83
removed
the
TODO:backport
Honor the lock order that transport lock > channel lock.
Resolves #2246