fix: in the swarm move Connectedness emit after releasing conns

[Jorropo]

go-libp2p-kad-dht now listen to both EvtPeerIdentificationCompleted and EvtPeerConnectednessChanged and EvtPeerIdentificationCompleted calls .ConnsToPeer inorder to do some filtering.

However it happens that it deadlocks because if the swarm is trying to emit a EvtPeerConnectednessChanged while the subscriber is trying to process an EvtPeerIdentificationCompleted, the subscriber is stuck on s.conns.RLock() while the swarm wont release it before having sent EvtPeerConnectednessChanged. Deadlock !

I havn't confirmed this fixes my bug given this takes time to reproduce, I'll startup a new experiment later.

[sukunrt]

See: #2373 (comment)

This is only a deadlock if your EvtPeerConnectednessChanged subscription channel is full, right?

Updated:
Okay no, I got it.
You cannot emit peer connectedness changed because the goroutine emitting event peer identification completed has the eventbus lock.
and the go routine emitting identification completed depends on the lock held by the other goroutine.

[Jorropo]

Yes, this has happened to me in the wild.

[sukunrt]

I agree this should be fixed, just want to get the scenario right.

• You have a subscription for both EvtPeerIdentificationCompleted and EvtPeerConnectednessChanged.
• You receive the identification completed event and try to obtain a swarm.conns.RLock
• The goroutine cannot acquire the lock because another goroutine has the lock which is going to push connectedness changed event. This goroutine cannot push the event because the queue of the same subscriber is full.

Does this sound correct?

[Jorropo]

@sukunrt what you described is exactly what happens right now because the emit is done while holding conns.
With my patch the goroutine will first release conns before emiting, so the RLock is free and taken by the consumer, the consumer then can continue to process events emptying the channel.
The only way my patch fail is if there are 2 goroutines trying to emit an event on the same stream, the first goroutine is stuck on emiting on a full channel the second one is blocked on notifylk and the consumer of the channel is blocked on conns (which hasn't been released yet) however I don't think that happens given previous releases works even tho the dht use notify (which is emited synchronously bellow already) and because notifylk is per stream not shared accross the swarm.

[MarcoPolo]

Great work @Jorropo! I didn't consider how a consumer could try to grab a lock that the publisher has and thus create a deadlock.

Publishers should only grab locks to guarantee the ordering of connect/disconnect events. I don't think it needs the swarm.conns lock, though I'll double check the code path.

I can help here and add some tests.

[Jorropo]

@MarcoPolo I don't have time to finish any of this, feel free to fix ci and add tests thx

[MarcoPolo]

Test added and fixed a related deadlock (on the disconnect side of things).

[MarcoPolo]

Once CI passes I'll merge a patch release

[Jorropo]

✔️ (can't approve) nice for the disconnect, I don't know how I overlooked this obvious codepath.