Fix old connections not being reused #1934
Conversation
tomaka
force-pushed
the
fix-not-tried-again
branch
from
41ac2d3 to
6f4bcb4
Compare
| trace!(target: "sub-libp2p", "Connect-to-nodes round; attempting to fill {:?} slots", | ||
| num_to_open); | ||
|
|
||
| // We first try to enable existing connections. | ||
| for peer_id in &self.connected_peers { | ||
| if num_to_open == 0 { |
There was a problem hiding this comment.
Shouldn't this if be outside at the outside of this for loop?
There was a problem hiding this comment.
Oh, that's because I forgot to add num_to_open -= 1; inside the loop.
| } | ||
|
|
||
| if let Some((_, expire)) = self.banned_peers.iter().find(|(p, _)| p == peer_id) { | ||
| if *expire >= Instant::now() { |
| continue; | ||
| } | ||
|
|
||
| if let Some((_, expire)) = self.banned_peers.iter().find(|(p, _)| p == peer_id) { |
There was a problem hiding this comment.
Just out of curiosity, do we hold connection to a peer that is banned?
There was a problem hiding this comment.
Yes. Banned peers can still perform Kademlia queries.
The connection should however close after a few seconds because it is marked as "useless".
| } | ||
|
|
||
| trace!(target: "sub-libp2p", "Enabling custom protocols with {:?} (active)", peer_id); | ||
| self.events.push(NetworkBehaviourAction::SendEvent { |
There was a problem hiding this comment.
If we enabled this "old" connection, do we still need to try to open a new connection?
There was a problem hiding this comment.
That doesn't open a new connection. It sends a message to the handler of the existing connection that it should enable it.
There was a problem hiding this comment.
What I really wanted to ask, shouldn't we return after sending this event?
There was a problem hiding this comment.
We continue looping until num_to_open is 0.
* Fix old connections not being reused * Address issues
When an issue happened on a connection, we would disable it.
However the
connect_to_nodesmethod doesn't take into account existing but disabled connections. The connection would keep existing but be totally useless.This PR fixes this.
#1777 is still there, but happens way less often.