Fix deadlock between close and reconnect

[Sean Pollock (seanavery)]

Description

Flaky test surfaced deadlock between reconnect and Close.

[Dan Gottlieb (dgottlieb)]

Benjamin Rewis (@benjirewis) can you and Sean Pollock (@seanavery) discuss this locking w.r.t active background workers and expectations around in flight requests during close?

[Benjamin Rewis (benjirewis)]

I haven't looked through this whole method/module, but I'd "normally" expect to see:

rc.closeMu.Lock()
rc.cancelFunc()
rc.closeMu.Unlock()
rc.activeBackgroundWorkers.Wait()

As a pattern for "request that all ongoing work stop and then wait for that ongoing work to actually stop." Let me know if you want to sync offline Sean Pollock (@seanavery). You'd have to walk me through what Close is trying to do here, but what you have currently looks a bit off.

[Sean Pollock (seanavery)]

hey Benjamin Rewis (@benjirewis) Dan Gottlieb (@dgottlieb), walking through my understanding of what was happening here

In the original Close:

rc.cancelFunc()
rc.closeMu.Lock()
rc.unsubscribeAll()
rc.activeBackgroundWorkers.Wait()

clientReconnectBackgroundWorker takes closeMu inside reconnectClient for each transport attempt. So it would block on the lock that Close was holding, while Close was blocked on Wait() waiting for that same worker to exit.

The test sets reconnectIntervalDuration = 10ms, which is why this surfaced so reliably. (Prod uses 5s)

The suggested Lock, cancel, Unlock, Wait pattern works nicely, the key thing is not holding the lock over Wait(). I didn't wrap cancelFunc() itself because context.CancelFunc is goroutine safe and the RWMutex around closeConnection() (the bit that does client.Close() rc.client = nil etc) already serializes teardown vs concurrent readers. Happy to switch to the canonical ordering for style, lmk.

[Benjamin Rewis (benjirewis)]

Sean and I spoke offline a bit about better usage of closeMu and cancelCtx + better documentation around the four mutexes on the rtspCamera struct.

[Sean Pollock (seanavery)]

Thanks Benjamin Rewis (@benjirewis). First pass at adopting your suggested pattern, and it seems to have turned out nicely

Bonus: holding closeMu over cancel let me wrap SubscribeRTP in RLock, which closed out a separate orphan subscription race and let me drop the mu dance around the FIR call.

[Benjamin Rewis (benjirewis)]

I'm surprised you're calling closeMu.Lock() twice in this function. Does everything below this line need closeMu held? If so, is there reason not to include it in the critical section above instead of this second one?

[Sean Pollock (seanavery)]

Yeah, not entirely happy with this, but it seems to be necessary for now:

• first lock makes cancelFunc atomic w the RLock holders
• cannot hold lock over Wait (reconnect loop deadlock)
• second lock is for general teardown

Lmk if a comment near the second lock would help

I think it is possible to refactor reconnect to not need the closeMu but seems out of scope for this PR.

[Sean Pollock (seanavery)]

apt commands are failing on arm64 runners right now:

Cannot initiate the connection to ports.ubuntu.com:80 (2620:2d:4002:1::10b). - connect (101: Network is unreachable) Cannot initiate the connection to ports.ubuntu.com:80 (2620:2d:4002:1::10c). - connect (101: Network is unreachable) Cannot initiate the connection to ports.ubuntu.com:80 (2620:2d:4002:1::10a). - connect (101: Network is unreachable) [IP: 91.189.91.104 80]

[Benjamin Rewis (benjirewis)]

I'm surprised you're calling closeMu.Lock() twice in this function. Does everything below this line need closeMu held? If so, is there reason not to include it in the critical section above instead of this second one?

[Benjamin Rewis (benjirewis)]

The usages of the closeMu certainly look more "normal" to me now, but I won't claim to have all the context to approve here. I'm still suspicious of the need to lock twice in Close, but your added documentation and the more consistent pattern in using/setting the client object make sense to me!