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.

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

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

Merged
merged 6 commits into from
Jul 2, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 3 additions & 0 deletions protocols/gossipsub/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3202,6 +3202,9 @@ where
NetworkBehaviourAction::ReportObservedAddr { address, score } => {
NetworkBehaviourAction::ReportObservedAddr { address, score }
}
NetworkBehaviourAction::DisconnectPeer { peer_id, handler } => {
NetworkBehaviourAction::DisconnectPeer { peer_id, handler }
}
});
}

Expand Down
4 changes: 3 additions & 1 deletion protocols/request-response/src/throttled.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,9 @@ where
| NetworkBehaviourAction::NotifyHandler { peer_id, handler, event } =>
NetworkBehaviourAction::NotifyHandler { peer_id, handler, event },
| NetworkBehaviourAction::ReportObservedAddr { address, score } =>
NetworkBehaviourAction::ReportObservedAddr { address, score }
NetworkBehaviourAction::ReportObservedAddr { address, score },
| NetworkBehaviourAction::DisconnectPeer { peer_id, handler } =>
NetworkBehaviourAction::DisconnectPeer { peer_id, handler }
};

return Poll::Ready(event)
Expand Down
3 changes: 3 additions & 0 deletions swarm-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,9 @@ fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> TokenStream {
std::task::Poll::Ready(#network_behaviour_action::ReportObservedAddr { address, score }) => {
return std::task::Poll::Ready(#network_behaviour_action::ReportObservedAddr { address, score });
}
std::task::Poll::Ready(#network_behaviour_action::DisconnectPeer { peer_id, handler }) => {
return std::task::Poll::Ready(#network_behaviour_action::DisconnectPeer { peer_id, handler });
}
std::task::Poll::Pending => break,
}
}
Expand Down
33 changes: 31 additions & 2 deletions swarm/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,15 @@ pub enum NetworkBehaviourAction<TInEvent, TOutEvent> {
/// relative to other observed addresses.
score: AddressScore,
},

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you and @thomaseizinger

/// The peer to disconnect.
peer_id: PeerId,
/// The ID of the connection whose `ProtocolsHandler` to disconnect.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

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

handler: DisconnectPeerHandler,
}
}

impl<TInEvent, TOutEvent> NetworkBehaviourAction<TInEvent, TOutEvent> {
Expand All @@ -312,7 +321,10 @@ impl<TInEvent, TOutEvent> NetworkBehaviourAction<TInEvent, TOutEvent> {
event: f(event)
},
NetworkBehaviourAction::ReportObservedAddr { address, score } =>
NetworkBehaviourAction::ReportObservedAddr { address, score }
NetworkBehaviourAction::ReportObservedAddr { address, score },
NetworkBehaviourAction::DisconnectPeer { peer_id, handler } => {
NetworkBehaviourAction::DisconnectPeer { peer_id, handler }
}
}
}

Expand All @@ -328,7 +340,9 @@ impl<TInEvent, TOutEvent> NetworkBehaviourAction<TInEvent, TOutEvent> {
NetworkBehaviourAction::NotifyHandler { peer_id, handler, event } =>
NetworkBehaviourAction::NotifyHandler { peer_id, handler, event },
NetworkBehaviourAction::ReportObservedAddr { address, score } =>
NetworkBehaviourAction::ReportObservedAddr { address, score }
NetworkBehaviourAction::ReportObservedAddr { address, score },
NetworkBehaviourAction::DisconnectPeer { peer_id, handler } =>
NetworkBehaviourAction::DisconnectPeer { peer_id, handler }
}
}
}
Expand Down Expand Up @@ -373,3 +387,18 @@ impl Default for DialPeerCondition {
DialPeerCondition::Disconnected
}
}

/// The options which connection handlers to disconnect.
Copy link
Member

Choose a reason for hiding this comment

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

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

#[derive(Debug, Clone)]
pub enum DisconnectPeerHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

/// Disconnect a particular connection handler.
One(ConnectionId),
/// Disconnect all connection handlers.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks!

All,
}

impl Default for DisconnectPeerHandler {
fn default() -> Self {
DisconnectPeerHandler::All
}
}
Loading