-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change to restarting of dead peers #239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put some comments. Also recently the e2e test was very stable, but it broke in this PR, which makes me think it has a bug
If you don't mind, I'll play more around this PR - remove first cancellation as it it clearly wrong now and look only on preventing retry on failed incoming peers. |
@@ -1018,6 +1020,12 @@ impl PeerHandler { | |||
|
|||
pe.value_mut().state.set(PeerState::Dead, &pstats); | |||
|
|||
if self.incoming { | |||
// do not retry incoming peers | |||
debug!("incoming peer {handle} died, not re-queueing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: at least for the sake of consistent style please put handle into a parameter, e.g. debug!(peer=handle, "incoming peer died, not re-queueing");
For a while I was observing how dead peers are retried and found couple of thing, which I think are sub-optimal:
on_peer_died
is called and eventually peer is scheduled as outgoing, TheSocketAddress
in this case has outgoing port of peer I think, so retry with it as outgoing does not make sense to me.This not final PR - rather a sketch to illustrate my findings. There is probably better way to solve it.