Add the ability for the behaviour to disconnect a peer #2110

sergeyboyko0791 · 2021-06-24T06:49:01Z

Currently, we use a custom version of the rust-libp2p crate in the https://github.com/KomodoPlatform/atomicDEX-API project that allows us to disconnect peers from the network behaviour.
I would like to propose this functionality to allow the users to build their custom behaviors with the ability to disconnect peers.
Please look at the example of using NetworkBehaviourAction::DisconnectPeer': https://github.com/KomodoPlatform/atomicDEX-API/blob/mm2.1/mm2src/mm2_libp2p/src/adex_ping.rs#L27

Add 'NetworkBehaviourAction::DisconnectPeer'
Add 'ExpandedSwarm::disconnect_peer_id'
Add 'test_swarm_disconnect', 'test_behaviour_disconnect_all', 'test_behaviour_disconnect_one'

* Add 'NetworkBehaviourAction::DisconnectPeer' * Add 'ExpandedSwarm::disconnect_peer_id' * Add 'test_swarm_disconnect', 'test_behaviour_disconnect_all', 'test_behaviour_disconnect_one'

thomaseizinger

I had a need for Swarm::disconnect_peer the other day and was surprised that it doesn't exist. I am in favor of this!

Implementation looks good to me, left some minor comments!

thomaseizinger · 2021-06-24T11:04:02Z

swarm/src/behaviour.rs

+    DisconnectPeer {
+        /// The peer to disconnect.
+        peer_id: PeerId,
+        /// The ID of the connection whose `ProtocolsHandler` to disconnect.


These docs seems to be slightly off given that DisconnectPeerHandler is not just the ID.

I see, thank you!

Suggested change

/// The ID of the connection whose `ProtocolsHandler` to disconnect.

/// Whether to close a specific or all connections to the given peer.

thomaseizinger · 2021-06-24T11:05:26Z

swarm/src/behaviour.rs

+
+/// The options which connection handlers to disconnect.
+#[derive(Debug, Clone)]
+pub enum DisconnectPeerHandler {


The docs mention the term Options, maybe DisconnectOptions is a better name for this?

I'd also agree to DisconnectPeer but DisconnectPeerHandler is a bit weird. In the end, we are disconnecting the actual peer, not the handler.

I suppose someone would like to disconnect a specified connection by its ConnectionId. DisconnectPeer may confuse in this case.

In the end, we are disconnecting the actual peer, not the handler.

I tried to follow the style of naming structures and enumerations that you follow :)
For example, here.

The docs mention the term Options, maybe DisconnectOptions is a better name for this?

I tried to follow the style of naming structures and enumerations that you follow :)

I did see that and thought you would have been inspired by NotifyHandler :)

What about CloseConnection::{One,All}? That naming would be inline with ProtocolsHandlerEvent::Close although that rename would affect the overall terminology used in PR.

I would be in favor of calling both the NetworkBehaviourAction CloseConnection (see my comment above) and this struct CloseConnection.

mxinden

First off, thanks @sergeyboyko0791 for the extensive pull request.

In general, I am not opposed to these changes.

I would like to use this opportunity to give users more guidance on which mechanism to use, now that we are adding yet another one. I guess guidance would be best given via additional documentation.

Today, as a user of libp2p-swarm, I see two ways to close a connection to a peer:

Via ProtocolsHandler::keep_alive in a collaborative manner across protocol handlers.
Via ProtocolsHandlerEvent::Close in a direct manner, instantly closing the connection whether used by other handlers or not.

Being able to close a connection from a NetworkBehaviour by emitting a NetworkBehaviourAction is a shortcut for (1). While I see this to be useful for people wanting to implement a Connection Manager via a NetworkBehaviour, I don't think this non-collaborative style of closing connections is suitable for other protocols like libp2p-kad or libp2p-gossipsub.

Now, in order to be able to give users guidance on which mechanism to use when, I would like to understand your use-case @sergeyboyko0791. Would you mind expanding on why you need to close a connection from a NetworkBehaviour?

I had a need for Swarm::disconnect_peer the other day and was surprised that it doesn't exist. I am in favor of this!

To get more data points, would you mind expanding on your use-case as well @thomaseizinger?

Please look at the example of using NetworkBehaviourAction::DisconnectPeer': https://github.com/KomodoPlatform/atomicDEX-API/blob/mm2.1/mm2src/mm2_libp2p/src/adex_ping.rs#L27

With the default PingConfig, libp2p-ping should close the connection on the second failure. Is that not the case for you? If so, would you mind either debugging this yourself or providing a reproducer?

mxinden · 2021-06-25T12:40:36Z

swarm/src/behaviour.rs

+    /// Instructs the `Swarm` to initiate a graceful close of the connection
+    /// with a peer.
+    DisconnectPeer {


Suggested change

/// Instructs the `Swarm` to initiate a graceful close of the connection

/// with a peer.

DisconnectPeer {

/// Instructs the `Swarm` to initiate a graceful close of one or all connections

/// with the given peer.

CloseConnection {

This naming would be more intuitive to me. What do you think @sergeyboyko0791?

Agree with you and @thomaseizinger

mxinden · 2021-06-25T12:51:52Z

swarm/src/behaviour.rs

+
+/// The options which connection handlers to disconnect.
+#[derive(Debug, Clone)]
+pub enum DisconnectPeerHandler {


I would be in favor of calling both the NetworkBehaviourAction CloseConnection (see my comment above) and this struct CloseConnection.

mxinden · 2021-06-25T12:52:43Z

swarm/src/lib.rs

@@ -463,6 +464,18 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
        self.banned_peers.remove(&peer_id);
    }

+    /// Disconnects a peer by its peer ID.


Suggested change

/// Disconnects a peer by its peer ID.

/// Disconnects a peer by its peer ID, closing all connections to said peer.

mxinden · 2021-06-25T12:54:21Z

swarm/src/behaviour.rs

+    /// Disconnect a particular connection handler.
+    One(ConnectionId),
+    /// Disconnect all connection handlers.


Suggested change

/// Disconnect a particular connection handler.

One(ConnectionId),

/// Disconnect all connection handlers.

/// Disconnect a particular connection.

One(ConnectionId),

/// Disconnect all connections.

Your changes closes the connection. The handler is not actively being closed.

I see, thanks!

thomaseizinger · 2021-06-27T09:56:35Z

To get more data points, would you mind expanding on your use-case as well @thomaseizinger?

My usecase were the examples for the rendezvous protocol. I wanted to showcase an application that:

connects to a rendezvous point
registers with the rendezvous point
disconnects from it
waits for incoming connections from other peers

To do this, I wanted an API on ExpandedSwarm analogous to dial_{peer, address} where I can disconnect from a specific peer.

For what it is worth, I think neither of the two existing approaches work very well here because the decision to disconnect is made on an application level. The individual NetworkBehaviours don't have enough knowledge to handle this.

sergeyboyko0791 · 2021-06-28T07:46:21Z

Hi @mxinden, thanks for your reply and suggestions.

With the default PingConfig, libp2p-ping should close the connection on the second failure. Is that not the case for you? If so, would you mind either debugging this yourself or providing a reproducer?

To be honest, I have not completely figured out how Ping works, and now I agree that it's not the use case to disconnect a peer manually from the behaviour.

Would you mind expanding on why you need to close a connection from a NetworkBehaviour?

We have one more example where we disconnect a peer using ExpandedSwarm::disconnect_peer_id instead of NetworkBehaviourAction::DisconnectPeer, but this logic can be moved to a behaviour:

We maintain the max number of connected relays regarding the GossipsubConfig::mesh_n_high value on every swarm poll. In theory, if this value was decreased externally (for example, on SIGHUP), we have to disconnect the extra relays.

Please, look at this.

* Rename 'NetworkBehaviourAction::DisconnectPeer' to 'NetworkBehaviourAction::CloseConnection' * Rename `DisconnectPeerHandler` to `CloseConnection`

sergeyboyko0791 · 2021-06-28T08:39:57Z

I've pushed the changes you requested. Could you please review it again?
cc @thomaseizinger @mxinden

mxinden · 2021-06-30T10:21:57Z

I've pushed the changes you requested. Could you please review it again?
cc @thomaseizinger @mxinden

Thanks. I will take a look likely later today.

mxinden

Thanks for writing test cases for each of the 3 new ways to disconnect a peer! Very much appreciated @sergeyboyko0791.

I have a couple smaller suggestions around documentation, in order to guide users. What do you think?

Also, could you please apply the diff below adding a changelog entry:

diff --git a/swarm/CHANGELOG.md b/swarm/CHANGELOG.md
index 103f88d2..0f01b314 100644
--- a/swarm/CHANGELOG.md
+++ b/swarm/CHANGELOG.md
@@ -15,7 +15,12 @@
 
   See [PR 2100] for details.
 
+- Add `ExpandedSwarm::disconnect_peer_id` and
+  `NetworkBehaviourAction::CloseConnection` to close connections to a specific
+  peer via an `ExpandedSwarm` or `NetworkBehaviour`. See [PR 2110] for details.
+
 [PR 2100]: https://github.com/libp2p/rust-libp2p/pull/2100
+[PR 2110]: https://github.com/libp2p/rust-libp2p/pull/2110/

mxinden · 2021-06-30T15:10:21Z

swarm/src/behaviour.rs

+        /// The peer to disconnect.
+        peer_id: PeerId,
+        /// The ID of the connection whose `ProtocolsHandler` to disconnect.
+        handler: CloseConnection,


Suggested change

handler: CloseConnection,

connection: CloseConnection,

mxinden · 2021-06-30T15:10:51Z

swarm/src/behaviour.rs

@@ -373,3 +387,18 @@ impl Default for DialPeerCondition {
        DialPeerCondition::Disconnected
    }
 }
+
+/// The options which connection handlers to disconnect.


Suggested change

/// The options which connection handlers to disconnect.

/// The options which connections to close.

swarm/src/behaviour.rs

mxinden · 2021-06-30T15:25:22Z

swarm/src/behaviour.rs

+    DisconnectPeer {
+        /// The peer to disconnect.
+        peer_id: PeerId,
+        /// The ID of the connection whose `ProtocolsHandler` to disconnect.


Suggested change

/// The ID of the connection whose `ProtocolsHandler` to disconnect.

/// Whether to close a specific or all connections to the given peer.

mxinden · 2021-06-30T15:30:36Z

swarm/src/lib.rs

@@ -463,6 +464,18 @@ where TBehaviour: NetworkBehaviour<ProtocolsHandler = THandler>,
        self.banned_peers.remove(&peer_id);
    }

+    /// Disconnects a peer by its peer ID, closing all connections to said peer.
+    ///
+    /// Returns `Ok(())` if a peer with this ID was in the list.


Suggested change

/// Returns `Ok(())` if a peer with this ID was in the list.

/// Returns `Ok(())` if there was one or more established connections to the peer

mxinden · 2021-06-30T15:43:16Z

swarm/src/lib.rs

+        }))
+    }
+
+    /// Establishes a number of connections between two peers,


Suggested change

/// Establishes a number of connections between two peers,

/// Establishes multiple connections between two peers,

mxinden · 2021-06-30T15:43:43Z

swarm/src/lib.rs

+
+    /// Establishes a number of connections between two peers,
+    /// after which one peer disconnects the other
+    /// using [`NetworkBehaviourAction::CloseConnection`] thrown from a behaviour.


Suggested change

/// using [`NetworkBehaviourAction::CloseConnection`] thrown from a behaviour.

/// using [`NetworkBehaviourAction::CloseConnection`] returned by a [`NetworkBehaviour`].

mxinden · 2021-06-30T15:44:48Z

swarm/src/lib.rs

@@ -1256,4 +1455,83 @@ mod tests {
            }
        }))
    }
+
+    /// Establishes a number of connections between two peers,


Suggested change

/// Establishes a number of connections between two peers,

/// Establishes multiple connections between two peers,

mxinden · 2021-06-30T15:45:16Z

swarm/src/lib.rs

+
+    /// Establishes a number of connections between two peers,
+    /// after which one peer closes the only one connection
+    /// using [`NetworkBehaviourAction::CloseConnection`] thrown from a behaviour.


Suggested change

/// using [`NetworkBehaviourAction::CloseConnection`] thrown from a behaviour.

/// using [`NetworkBehaviourAction::CloseConnection`] returned by a [`NetworkBehaviour`].

mxinden · 2021-06-30T15:48:14Z

swarm/src/lib.rs

@@ -1256,4 +1455,83 @@ mod tests {
            }
        }))
    }
+
+    /// Establishes a number of connections between two peers,
+    /// after which one peer closes the only one connection


Suggested change

/// after which one peer closes the only one connection

/// after which one peer closes a single connection

* Rename 'NetworkBehaviourAction::CloseConnection::handler' to 'NetworkBehaviourAction::CloseConnection::connection'

sergeyboyko0791 · 2021-07-02T12:50:08Z

@mxinden, all done.
I just formatted the comments.
Thanks for the detailed review 🙃

swarm/src/behaviour.rs

swarm/src/lib.rs

sergeyboyko0791 · 2021-07-02T15:47:10Z

I almost prepared the fix :D

sergeyboyko0791 · 2021-07-03T05:54:53Z

Thank you @mxinden @thomaseizinger for the review!

Add the ability for the behaviour to disconnect a peer

5bbebb6

* Add 'NetworkBehaviourAction::DisconnectPeer' * Add 'ExpandedSwarm::disconnect_peer_id' * Add 'test_swarm_disconnect', 'test_behaviour_disconnect_all', 'test_behaviour_disconnect_one'

thomaseizinger approved these changes Jun 24, 2021

View reviewed changes

mxinden reviewed Jun 25, 2021

View reviewed changes

Improve documentation and structure naming

9db53b7

* Rename 'NetworkBehaviourAction::DisconnectPeer' to 'NetworkBehaviourAction::CloseConnection' * Rename `DisconnectPeerHandler` to `CloseConnection`

thomaseizinger mentioned this pull request Jun 28, 2021

Ping protocol closes connection if other party doesn't support /ping #2109

Closed

mxinden reviewed Jun 30, 2021

View reviewed changes

sergeyboyko0791 added 2 commits July 2, 2021 19:42

Improve documentation and the code

abb23d9

* Rename 'NetworkBehaviourAction::CloseConnection::handler' to 'NetworkBehaviourAction::CloseConnection::connection'

Commit the changes in CHANGELOG.md

7c43788

Merge branch 'master' into disconnect-peer

1ac40e6

mxinden reviewed Jul 2, 2021

View reviewed changes

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

swarm/src/lib.rs Outdated Show resolved Hide resolved

swarm/src: Fix doc comment links

cc61123

mxinden approved these changes Jul 2, 2021

View reviewed changes

mxinden merged commit 4eb0659 into libp2p:master Jul 2, 2021

This was referenced Jul 4, 2021

Implement NetDisconnect ChainSafe/forest#1181

Closed

Update libp2p to 0.40 ChainSafe/forest#1182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability for the behaviour to disconnect a peer #2110

Add the ability for the behaviour to disconnect a peer #2110

sergeyboyko0791 commented Jun 24, 2021

thomaseizinger left a comment

thomaseizinger Jun 24, 2021

sergeyboyko0791 Jun 24, 2021

mxinden Jun 30, 2021

thomaseizinger Jun 24, 2021

sergeyboyko0791 Jun 24, 2021

thomaseizinger Jun 25, 2021

mxinden Jun 25, 2021

mxinden left a comment

mxinden Jun 25, 2021

sergeyboyko0791 Jun 28, 2021

mxinden Jun 25, 2021

mxinden Jun 25, 2021

sergeyboyko0791 Jun 28, 2021

mxinden Jun 25, 2021

sergeyboyko0791 Jun 28, 2021

thomaseizinger commented Jun 27, 2021

sergeyboyko0791 commented Jun 28, 2021

sergeyboyko0791 commented Jun 28, 2021

mxinden commented Jun 30, 2021

mxinden left a comment

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

mxinden Jun 30, 2021

sergeyboyko0791 commented Jul 2, 2021

sergeyboyko0791 commented Jul 2, 2021

sergeyboyko0791 commented Jul 3, 2021

	/// The ID of the connection whose `ProtocolsHandler` to disconnect.
	/// Whether to close a specific or all connections to the given peer.

	/// Disconnects a peer by its peer ID.
	/// Disconnects a peer by its peer ID, closing all connections to said peer.

	/// The options which connection handlers to disconnect.
	/// The options which connections to close.

	/// Returns `Ok(())` if a peer with this ID was in the list.
	/// Returns `Ok(())` if there was one or more established connections to the peer

	/// Establishes a number of connections between two peers,
	/// Establishes multiple connections between two peers,

	/// using [`NetworkBehaviourAction::CloseConnection`] thrown from a behaviour.
	/// using [`NetworkBehaviourAction::CloseConnection`] returned by a [`NetworkBehaviour`].

	/// after which one peer closes the only one connection
	/// after which one peer closes a single connection

Add the ability for the behaviour to disconnect a peer #2110

Add the ability for the behaviour to disconnect a peer #2110

Conversation

sergeyboyko0791 commented Jun 24, 2021

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Jun 27, 2021

sergeyboyko0791 commented Jun 28, 2021

sergeyboyko0791 commented Jun 28, 2021

mxinden commented Jun 30, 2021

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 commented Jul 2, 2021

sergeyboyko0791 commented Jul 2, 2021

sergeyboyko0791 commented Jul 3, 2021