Fix concurrency and silence period not being honoured

[raulk]

Fixes #23. When connections are added while we're above the high watermark, we trigger multiple concurrent trims at once. This patch addresses these problems:

• The silence period was not honoured because we don't read lastTrim inside a lock; hence trims were allowed to proceed when they shouldn't have.
• If all connections were scored the same (e.g. zero), each concurrent prune would end up trimming random connections (as a result of map iteration randomness), meaning that there's a high likelihood (captured in the test) that we end up pruning more connections than we should've (probably all of them!).
• As multiple trims were running concurrently, they were closing overlapping subsets of connections, and the disconnect notifee ended up being called more than once per connection (visible in the test now that I've wired the disconnect notifee; run this new test against the old version of the conn manager to see the warnings).

[Stebalien]

As multiple trims were running concurrently, they were closing overlapping subsets of connections, and the disconnect notifee is called more than once per connection

You mean Close was getting called multiple times, right? The swarm disconnect notifiee shouldn't get called more than once per connection no matter how many times Close is called.

[Stebalien]

But yeah... this would definitely help explain some bitswap reliability issues.

[raulk]

You mean Close was getting called multiple times, right? The swarm disconnect notifiee shouldn't get called more than once per connection no matter how many times Close is called.

True, we should fix that at the origin. In this case, the dup notification has no impact other than a warning log, but for pubsub it does. @vyzo just filed this today as well: libp2p/go-libp2p-pubsub#130, stemming from vacp2p/nim-libp2p#8

But yeah... this would definitely help explain some bitswap reliability issues.

Agree. In general, bursty protocols trigger the worst case scenario.

[Stebalien]

Thanks for catching this! I'd like to consider the semantics of TrimOpenConns a bit but that shouldn't block this fix.

[Stebalien]

True, we should fix that at the origin. In this case, the dup notification has no impact other than a warning log, but for pubsub it does. @vyzo just filed this today as well: libp2p/go-libp2p-pubsub#130, stemming from vacp2p/nim-libp2p#8

I think that's a different issue. That is, we might have multiple connections for a single peer and will trigger a separate disconnect notification per connection.

That's why I asked, I'm pretty sure we're not firing multiple disconnect notifications for a single connection.

[vyzo]

Yeah, the pubsub issue is that we have broken peer management -- it's not designed to handle multiple connections, and it breaks as soon as one of the connections is closed.