Fix connected peers bombarding peers with connection attempts #2208
Conversation
|
It'd be great if you could write test for this edge case as well, @shamil-gadelshin |
nazar-pc
force-pushed
the
fix-connected-peers-bursts
branch
from
a938ca1
to
73874a0
Compare
|
Removed one unnecessary waker call |
| @@ -508,7 +529,9 @@ impl<Instance: 'static + Send> NetworkBehaviour for Behaviour<Instance> { | |||
|
|
|||
| for (peer_id, address) in peer_addresses { | |||
| self.known_peers.entry(peer_id).or_insert_with(|| { | |||
| ConnectionState::Connecting { | |||
| cx.waker().wake_by_ref(); | |||
There was a problem hiding this comment.
I wonder, will it work while we process the current poll()?
There was a problem hiding this comment.
I would be VERY surprised if it didn't. This is how futures work and how this should work as well.
| @@ -508,7 +529,9 @@ impl<Instance: 'static + Send> NetworkBehaviour for Behaviour<Instance> { | |||
|
|
|||
| for (peer_id, address) in peer_addresses { | |||
| self.known_peers.entry(peer_id).or_insert_with(|| { | |||
| ConnectionState::Connecting { | |||
| cx.waker().wake_by_ref(); | |||
There was a problem hiding this comment.
I would be VERY surprised if it didn't. This is how futures work and how this should work as well.
Since we don't have either manual testing or unit testing I would postpone the PR until we perform some form of testing. Otherwise, we introduce un untested major change to the protocol. |
nazar-pc
force-pushed
the
fix-connected-peers-bursts
branch
from
73874a0
to
41c9880
Compare
The first commit is just some logging tweaks, some formatting and some to debug what is going on better.
The second commit fixes following thing I observed in logs (note the time difference between lines):
Essentially the way
fn pollwas written the following happened:ConnectionState::Connecting(but waker wasn't called, so there was no guarantee it will ever actually dial that peer)ConnectionState::ConnectingThis change both fixes missing waker call and introduces
ConnectionState::ToConnectthat is used to indicate that connection should be initiated, but it didn't actually happen yet.This doesn't fix networking issues I observe fully, but this seems like a clear bug that should be fixed anyway.
Code contributor checklist: