stub: cancel Start() for early connection loss.#238
Conversation
| func (stub *stub) connClosed() { | ||
| select { | ||
| // if our connection gets closed before we get Configure()'d, let Start() know | ||
| case stub.cfgErrC <- ttrpc.ErrClosed: |
There was a problem hiding this comment.
since we are overloading the cfgErrC is there any chance we end up with two writers to the channel in one Start?
There was a problem hiding this comment.
So if connClosed writes to cfgErrc while Configure is running, the deferred send in Configure would block forever and leak a goroutine?
What if we make the send in Configure non-blocking as well? That would ensure whichever function gets there first wins.
defer func() {
select {
case stub.cfgErrC <- retErr:
default:
}
}()What do you think?
There was a problem hiding this comment.
I am not sure, because I don't know (and did not try to check/test) how racy a socket connection close soon after a ttrpc message sending over the same socket can end up being wrt. ttrpc delivering the message or an onClose() callback. Therefore I wanted to err on the side of safety, so although cfgErrC is a buffered channel with a capacity of 1, I still decided to do an attempted/non-blocking send here with a select, so we can't get stuck here. If sending here fails, it means that there is already a pending/unreceived cfgErr in the channel, which will nudge Start() out of the wait-receive, so no harm is done if the extra ttrpc.ErrClosed attempted to send here is lost. And if Configure() has not failed yet, or the failure error has not been delivered over the channel yet, sending will succeed as the channel is empty, which again should nudge Start() out of the wait-receive.
There was a problem hiding this comment.
So if
connClosedwrites tocfgErrcwhileConfigureis running, the deferred send inConfigurewould block forever and leak a goroutine?
Hmm, I think it shouldn't. The channel is buffered with a capacity of 1. We always read/receive 1 error, sent from Configure() or connClosed(), whichever comes first, and we try to send at most 2. So if we send 2, then 1 stays buffered in the channel, which should not be a problem either, because if the stub is ever re-Start()ed (so we try to go through this again), it creates a new cfgErrC channel.
What if we make the send in
Configurenon-blocking as well? That would ensure whichever function gets there first wins.defer func() { select { case stub.cfgErrC <- retErr: default: } }()What do you think?
Yes, I think that is a good idea. I updated the PR accordingly.
klihub
force-pushed
the
fixes/cancel-start-on-early-conn-loss
branch
from
bf9edb1 to
d8515b0
Compare
If our connection gets closed before we had a chance to get Configure()'d by the runtime, cancel Start()'s wait for the result by letting it know about the failure. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
klihub
force-pushed
the
fixes/cancel-start-on-early-conn-loss
branch
from
d8515b0 to
1681e81
Compare
|
Closing this for now, will need to take a closer look. |
Start() holds stub.Lock() for its entire body and blocks on <-cfgErrC waiting for the runtime's Configure() callback. If the runtime accepts RegisterPlugin but never sends Configure() (observed when a plugin and containerd race during node boot), Start() blocks forever holding the lock. When the connection later closes, the ttrpc OnClose handler connClosed() tries to take the same lock and deadlocks, so the plugin never observes the disconnect and cannot recover. Fix by: - replacing the naked <-cfgErrC receive with a select that also honours ctx cancellation and a registrationTimeout-bounded timer; - having connClosed() do a non-blocking send of a ttrpc.ErrClosed-wrapped error on cfgErrC before taking the lock, so a blocked Start() is woken and releases the lock; - making Configure()'s deferred cfgErrC send non-blocking so it cannot wedge if Start() has already given up or connClosed() has already filled the buffer. Add a regression test that lets Register succeed, blocks the plugin's Configure handler so cfgErrC is never written, then has the runtime time out and close the connection; without this fix Start() never returns. Supersedes containerd#238. Signed-off-by: Chris Evans <wevans@anthropic.com>
Start() holds stub.Lock() for its entire body and blocks on <-cfgErrC waiting for the runtime's Configure() callback. If the runtime accepts RegisterPlugin but never sends Configure() (observed when a plugin and containerd race during node boot), Start() blocks forever holding the lock. When the connection later closes, the ttrpc OnClose handler connClosed() tries to take the same lock and deadlocks, so the plugin never observes the disconnect and cannot recover. Fix by: - replacing the naked <-cfgErrC receive with a select that also honours ctx cancellation and a registrationTimeout-bounded timer; - having connClosed() do a non-blocking send of a ttrpc.ErrClosed-wrapped error on cfgErrC before taking the lock, so a blocked Start() is woken and releases the lock; - making Configure()'s deferred cfgErrC send non-blocking so it cannot wedge if Start() has already given up or connClosed() has already filled the buffer. Add a regression test that lets Register succeed, blocks the plugin's Configure handler so cfgErrC is never written, then has the runtime time out and close the connection; without this fix Start() never returns. Supersedes containerd#238. Signed-off-by: Chris Evans <wevans@anthropic.com>
Start() holds stub.Lock() for its entire body and blocks on <-cfgErrC waiting for the runtime's Configure() callback. If the runtime accepts RegisterPlugin but never sends Configure() (observed when a plugin and containerd race during node boot), Start() blocks forever holding the lock. When the connection later closes, the ttrpc OnClose handler connClosed() tries to take the same lock and deadlocks, so the plugin never observes the disconnect and cannot recover. Fix by: - replacing the naked <-cfgErrC receive with a select that also honours ctx cancellation and a registrationTimeout-bounded timer; - having connClosed() do a non-blocking send of a ttrpc.ErrClosed-wrapped error on cfgErrC before taking the lock, so a blocked Start() is woken and releases the lock; - making Configure()'s deferred cfgErrC send non-blocking so it cannot wedge if Start() has already given up or connClosed() has already filled the buffer. To keep this race-clean under -race: - snapshot stub.registrationTimeout into a local before any goroutines are running, since Configure() rewrites that field without the lock; - create srvErrC/cfgErrC before ttrpc.NewClient spawns the goroutine whose OnClose reads cfgErrC without the lock (this is the race that containerd#238's test surfaced). Add two adaptation-suite regression specs: one that lets Register succeed and blocks the plugin's Configure handler so cfgErrC is never written, then has the runtime time out and close the connection (deterministic deadlock repro); and one that has the runtime drop the connection during registration (surfaces the cfgErrC field race under -race). Supersedes containerd#238. Signed-off-by: Chris Evans <wevans@anthropic.com>
3 participants
If our connection gets closed before we had a chance to get Configure()'d by the runtime, cancel Start()'s wait for the result by letting it know about the failure. Otherwise the stub might get stuck in Start().