Skip to content
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.

Sign up for GitHub

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

Make sure inject_dial_failure is called #1549

Merged
merged 9 commits into from
Apr 17, 2020
Merged

Make sure inject_dial_failure is called #1549

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6ad1fb8
Make sure inject_dial_failure is called
tomaka Apr 17, 2020
b385b6e
Update CHANGELOG
tomaka Apr 17, 2020
6de49ea
Merge branch 'master' into fix-dial
tomaka Apr 17, 2020
c10ef27
Make libp2p-kad tests pass
tomaka Apr 17, 2020
80c0d3d
Fix again
tomaka Apr 17, 2020
5765095
Update swarm/src/lib.rs
tomaka Apr 17, 2020
75346c1
Revert "Fix again"
tomaka Apr 17, 2020
c465829
Bump versions and CHANGELOG
tomaka Apr 17, 2020
54a432d
Oops, didn't bump libp2p-swarm in libp2p
tomaka Apr 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# Version ???

- `libp2p-swarm`: Make sure inject_dial_failure is called in all situations.
[PR 1549](https://github.com/libp2p/rust-libp2p/pull/1549)

# Version 0.18.0 (2020-04-09)

Expand Down
54 changes: 32 additions & 22 deletions swarm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,12 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
return Err(error)
}
}
} else {
log::debug!(
"New dialing attempt to disconnected peer {:?} failed: no address.",
peer_id
);
me.behaviour.inject_dial_failure(&peer_id);
}
Ok(false)
},
Expand All @@ -419,6 +425,12 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
return Err(error)
}
}
} else {
log::debug!(
"New dialing attempt to disconnected peer {:?} failed: no address.",
peer_id
);
me.behaviour.inject_dial_failure(&peer_id);
}
Ok(false)
}
Expand All @@ -427,6 +439,7 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
Ok(false)
},
Peer::Local => {
me.behaviour.inject_dial_failure(&peer_id);
Err(ConnectionLimit { current: 0, limit: 0 })
}
}
Expand Down Expand Up @@ -701,34 +714,31 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
if this.banned_peers.contains(&peer_id) {
this.behaviour.inject_dial_failure(&peer_id);
} else {
let result = match condition {
let condition_matched = match condition {
DialPeerCondition::Disconnected
if this.network.is_disconnected(&peer_id) =>
{
ExpandedSwarm::dial(this, &peer_id)
}
if this.network.is_disconnected(&peer_id) => true,
DialPeerCondition::NotDialing
if !this.network.is_dialing(&peer_id) =>
{
ExpandedSwarm::dial(this, &peer_id)
if !this.network.is_dialing(&peer_id) => true,
_ => false
};

if condition_matched {
if let Ok(true) = ExpandedSwarm::dial(this, &peer_id) {
return Poll::Ready(SwarmEvent::Dialing(peer_id));
}
_ => {
log::trace!("Condition for new dialing attempt to {:?} not met: {:?}",
peer_id, condition);
if let Some(mut peer) = this.network.peer(peer_id.clone()).into_dialing() {

} else {
log::trace!("Condition for new dialing attempt to {:?} not met: {:?}",
peer_id, condition);
match this.network.peer(peer_id.clone()) {
Peer::Dialing(mut peer) => {
let addrs = this.behaviour.addresses_of_peer(peer.id());
peer.connection().add_addresses(addrs);
},
Peer::Connected(_) => {},
Peer::Disconnected(..) | Peer::Local => {
this.behaviour.inject_dial_failure(&peer_id);
Copy link
Contributor

@romanb romanb Apr 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not sure about calling inject_dial_failure when the dialing condition was not met. I would only expect a call to inject_dial_failure if my behaviour emitted DialPeer and the dialing condition was actually met, since by specifying the condition I'm saying under which conditions I actually want to dial and wouldn't expect to be informed about a dial failure if I didn't actually intend to dial as per my condition. Admittedly, as I wrote in #1506, if a behaviour actually wants to know if the dialing condition is not met because it would otherwise still keep some state, another callback may be needed then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be ok if we clearly document that inject_dial_failure is called also if the dialing condition is not met, i.e. such that every DialPeer is guaranteed to be paired with inject_connection_established or inject_dial_failure, but then there should be no exception for when the peer is already connected, again because the behaviour may otherwise keep some state around.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can revert that.

}
Ok(false)
}
};
match result {
Ok(false) => {},
Ok(true) => return Poll::Ready(SwarmEvent::Dialing(peer_id)),
Err(err) => {
log::debug!("Initiating dialing attempt to {:?} failed: {:?}",
&peer_id, err);
this.behaviour.inject_dial_failure(&peer_id);
}
}
}
Expand Down