Fixed bug with disconnection and max_non_reserved_peers
#974
Conversation
| // disconnect the surplus peer | ||
| let _ = self.swarm.disconnect_peer_id(peer_to_disconnect); | ||
| // reconnect the reserved peer |
There was a problem hiding this comment.
We already have a health timer that connects to disconnected reserved peers. We don't need to duplicate this work here=)
| if other_established == 0 { | ||
| // this is the first connection to a given Peer | ||
| self.peer_manager.handle_initial_connection(peer_id); | ||
| } | ||
|
|
||
| let addresses = self.addresses_of_peer(&peer_id); |
There was a problem hiding this comment.
First, we need to insert the peer into the table in the handle_initial_connection, and after we can add peer addresses=)
| // Too many peers already connected, disconnect the Peer | ||
| self.pending_events.push_back(PeerInfoEvent::TooManyPeers { | ||
| // Too many peers already connected, disconnect the Peer with the first priority. | ||
| self.pending_events.push_front(PeerInfoEvent::TooManyPeers { |
There was a problem hiding this comment.
I think we need to disconnect from peers in the first place. Otherwise, we will have a pending task that will receive incoming messages that we plan to drop.
| @@ -645,28 +636,33 @@ impl PeerManager { | |||
| fn handle_peer_disconnect(&mut self, peer_id: PeerId) { | |||
| // try immediate reconnect if it's a reserved peer | |||
| let is_reserved = self.reserved_peers.contains(&peer_id); | |||
| let is_removed; | |||
There was a problem hiding this comment.
In the handle_initial_connection we don't connect to the peer -> it absents in our self.non_reserved_connected_peers map.
We had a bug here that we will allow_new_peers even if the peer was not connected -> we allowed new connections when we had max_non_reserved_peers.
xgreenx
changed the title
xgreenx
changed the title
max_non_reserved_peers
| @@ -876,7 +869,7 @@ mod tests { | |||
| let jh = tokio::spawn(async move { | |||
| while rx.try_recv().is_err() { | |||
| futures::stream::iter(node_services.iter_mut()) | |||
| .for_each_concurrent(10, |node| async move { | |||
| .for_each_concurrent(20, |node| async move { | |||
There was a problem hiding this comment.
Should this at least be a const value?
During #974 optimistic reconnect was removed from the code as being a _duplicate_ work. It was not a duplicate work, more of an optimistic immediate reconnect if the node is available, while `health_check` tries to reconnect after a certain Interval (e.g. 30-60 seconds) which is there as a backup only if the optimistic reconnect was not successful. I also added a small description comment to `OutboundState`.
Closes #967