Skip to content

Commit

Permalink
feat(swarm): allow both Disconnected and NotDialing condition combined
Browse files Browse the repository at this point in the history
Dialing by default uses the condition `Disconnected`, but it seems appropriate to additionally prevent a dial if already dialing. This means a combination of `Disconnected` and `NotDialing`.

The default is changed to a new variant `DisconnectedAndNotDialing`. This is a breaking change.

Related: #4189.

Pull-Request: #4225.
  • Loading branch information
b-zee authored Oct 20, 2023
1 parent 3c00ae8 commit 51262c7
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 11 deletions.
12 changes: 10 additions & 2 deletions misc/connection-limits/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,8 @@ impl NetworkBehaviour for Behaviour {
mod tests {
use super::*;
use libp2p_swarm::{
behaviour::toggle::Toggle, dial_opts::DialOpts, DialError, ListenError, Swarm, SwarmEvent,
behaviour::toggle::Toggle, dial_opts::DialOpts, dial_opts::PeerCondition, DialError,
ListenError, Swarm, SwarmEvent,
};
use libp2p_swarm_test::SwarmExt;
use quickcheck::*;
Expand All @@ -401,14 +402,21 @@ mod tests {
network
.dial(
DialOpts::peer_id(target)
// Dial always, even if already dialing or connected.
.condition(PeerCondition::Always)
.addresses(vec![addr.clone()])
.build(),
)
.expect("Unexpected connection limit.");
}

match network
.dial(DialOpts::peer_id(target).addresses(vec![addr]).build())
.dial(
DialOpts::peer_id(target)
.condition(PeerCondition::Always)
.addresses(vec![addr])
.build(),
)
.expect_err("Unexpected dialing success.")
{
DialError::Denied { cause } => {
Expand Down
9 changes: 8 additions & 1 deletion misc/memory-connection-limits/tests/max_bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ fn max_bytes() {
network
.dial(
DialOpts::peer_id(target)
// Always dial, even if connected or already dialing.
.condition(libp2p_swarm::dial_opts::PeerCondition::Always)
.addresses(vec![addr.clone()])
.build(),
)
Expand All @@ -70,7 +72,12 @@ fn max_bytes() {
std::thread::sleep(Duration::from_millis(100)); // Memory stats are only updated every 100ms internally, ensure they are up-to-date when we try to exceed it.

match network
.dial(DialOpts::peer_id(target).addresses(vec![addr]).build())
.dial(
DialOpts::peer_id(target)
.condition(libp2p_swarm::dial_opts::PeerCondition::Always)
.addresses(vec![addr])
.build(),
)
.expect_err("Unexpected dialing success.")
{
DialError::Denied { cause } => {
Expand Down
14 changes: 12 additions & 2 deletions misc/memory-connection-limits/tests/max_percentage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ use std::time::Duration;
use sysinfo::{RefreshKind, SystemExt};
use util::*;

use libp2p_swarm::{dial_opts::DialOpts, DialError, Swarm};
use libp2p_swarm::{
dial_opts::{DialOpts, PeerCondition},
DialError, Swarm,
};
use libp2p_swarm_test::SwarmExt;

#[test]
Expand Down Expand Up @@ -63,6 +66,8 @@ fn max_percentage() {
network
.dial(
DialOpts::peer_id(target)
// Always dial, even if already dialing or connected.
.condition(PeerCondition::Always)
.addresses(vec![addr.clone()])
.build(),
)
Expand All @@ -72,7 +77,12 @@ fn max_percentage() {
std::thread::sleep(Duration::from_millis(100)); // Memory stats are only updated every 100ms internally, ensure they are up-to-date when we try to exceed it.

match network
.dial(DialOpts::peer_id(target).addresses(vec![addr]).build())
.dial(
DialOpts::peer_id(target)
.condition(PeerCondition::Always)
.addresses(vec![addr])
.build(),
)
.expect_err("Unexpected dialing success.")
{
DialError::Denied { cause } => {
Expand Down
4 changes: 3 additions & 1 deletion protocols/kad/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1995,7 +1995,9 @@ where
}
}
DialError::DialPeerConditionFalse(
dial_opts::PeerCondition::Disconnected | dial_opts::PeerCondition::NotDialing,
dial_opts::PeerCondition::Disconnected
| dial_opts::PeerCondition::NotDialing
| dial_opts::PeerCondition::DisconnectedAndNotDialing,
) => {
// We might (still) be connected, or about to be connected, thus do not report the
// failure to the queries.
Expand Down
4 changes: 4 additions & 0 deletions swarm/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
## 0.44.0 - unreleased

- Add `PeerCondition::DisconnectedAndNotDialing` variant, combining pre-existing conditions.
This is the new default.
A new dialing attempt is iniated _only if_ the peer is both considered disconnected and there is currently no ongoing dialing attempt.
See [PR 4225](https://github.com/libp2p/rust-libp2p/pull/4225).
- Remove deprecated `keep_alive_timeout` in `OneShotHandlerConfig`.
See [PR 4677](https://github.com/libp2p/rust-libp2p/pull/4677).

Expand Down
10 changes: 7 additions & 3 deletions swarm/src/dial_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,14 +311,18 @@ impl WithoutPeerIdWithAddress {
#[derive(Debug, Copy, Clone, Default)]
pub enum PeerCondition {
/// A new dialing attempt is initiated _only if_ the peer is currently
/// considered disconnected, i.e. there is no established connection
/// and no ongoing dialing attempt.
#[default]
/// considered disconnected, i.e. there is no established connection.
Disconnected,
/// A new dialing attempt is initiated _only if_ there is currently
/// no ongoing dialing attempt, i.e. the peer is either considered
/// disconnected or connected but without an ongoing dialing attempt.
NotDialing,
/// A combination of [`Disconnected`](PeerCondition::Disconnected) and
/// [`NotDialing`](PeerCondition::NotDialing). A new dialing attempt is
/// iniated _only if_ the peer is both considered disconnected and there
/// is currently no ongoing dialing attempt.
#[default]
DisconnectedAndNotDialing,
/// A new dialing attempt is always initiated, only subject to the
/// configured connection limits.
Always,
Expand Down
7 changes: 5 additions & 2 deletions swarm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,13 @@ where
let connection_id = dial_opts.connection_id();

let should_dial = match (condition, peer_id) {
(_, None) => true,
(PeerCondition::Always, _) => true,
(PeerCondition::Disconnected, None) => true,
(PeerCondition::NotDialing, None) => true,
(PeerCondition::Disconnected, Some(peer_id)) => !self.pool.is_connected(peer_id),
(PeerCondition::NotDialing, Some(peer_id)) => !self.pool.is_dialing(peer_id),
(PeerCondition::DisconnectedAndNotDialing, Some(peer_id)) => {
!self.pool.is_dialing(peer_id) && !self.pool.is_connected(peer_id)
}
};

if !should_dial {
Expand Down Expand Up @@ -1742,6 +1744,7 @@ impl fmt::Display for DialError {
),
DialError::DialPeerConditionFalse(PeerCondition::Disconnected) => write!(f, "Dial error: dial condition was configured to only happen when disconnected (`PeerCondition::Disconnected`), but node is already connected, thus cancelling new dial."),
DialError::DialPeerConditionFalse(PeerCondition::NotDialing) => write!(f, "Dial error: dial condition was configured to only happen if there is currently no ongoing dialing attempt (`PeerCondition::NotDialing`), but a dial is in progress, thus cancelling new dial."),
DialError::DialPeerConditionFalse(PeerCondition::DisconnectedAndNotDialing) => write!(f, "Dial error: dial condition was configured to only happen when both disconnected (`PeerCondition::Disconnected`) and there is currently no ongoing dialing attempt (`PeerCondition::NotDialing`), but node is already connected or dial is in progress, thus cancelling new dial."),
DialError::DialPeerConditionFalse(PeerCondition::Always) => unreachable!("Dial peer condition is by definition true."),
DialError::Aborted => write!(
f,
Expand Down

0 comments on commit 51262c7

Please sign in to comment.