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

Jump to bottom

Banning a connected peer does not result in calls to NetworkBehaviour methods #1584

Closed
koivunej opened this issue May 19, 2020 · 1 comment
Closed

Banning a connected peer does not result in calls to NetworkBehaviour methods #1584

koivunej opened this issue May 19, 2020 · 1 comment

Comments

@koivunej
Copy link
Contributor

Banning a connected peer with ExpandedSwarm::ban_peer_id does not seem to result in any NetworkBehaviour method calls by the swarm on the banning peer side. On the banned peer the closed connection(s) and disconnected peer are forwarded to NetworkBehaviour's respective methods.

Can't see any changes to the behaviour in #1440 and the original #1065 which introduced the ban/unban feature seems to work in a similar way. I think the expected behaviour would be to notify the swarm's NetworkBehaviour of the disconnection to allow it/them to cleanup any possible state they may have had regarding the peer. This could probably be done through inject_connection_closed and finally inject_disconnected.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jun 19, 2020
Emit events for active connection close and fix disconnect().
6c2b546 
The `Network` does currently not emit events for actively
closed connections, e.g. via `EstablishedConnection::close`
or `ConnectedPeer::disconnect()`. As a result, when actively
closing connections, there will be `ConnectionEstablished`
events emitted without eventually a matching `ConnectionClosed`
event. This seems undesirable and has the consequence that
the `Swarm::ban_peer_id` feature in `libp2p-swarm` does not
result in appropriate calls to `NetworkBehaviour::inject_connection_closed`
and `NetworkBehaviour::inject_disconnected`. Furthermore,
the `disconnect()` functionality in `libp2p-core` is currently
broken as it leaves the `Pool` in an inconsistent state.

This commit does the following:

  1. When connection background tasks are dropped
     (i.e. removed from the `Manager`), they
     always terminate immediately, without attempting
     an orderly close of the connection.
  2. An orderly close is sent to the background task
     of a connection as a regular command. The
     background task emits a `Closed` event
     before terminating.
  3. `Pool::disconnect()` removes all connection
     tasks for the affected peer from the `Manager`,
     i.e. without an orderly close, thereby also
     fixing the discovered state inconsistency
     due to not removing the corresponding entries
     in the `Pool` itself after removing them from
     the `Manager`.
  4. A new test is added to `libp2p-swarm` that
     exercises the ban/unban functionality and
     places assertions on the number and order
     of calls to the `NetworkBehaviour`. In that
     context some new testing utilities have
     been added to `libp2p-swarm`.

This addresses libp2p#1584.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jun 19, 2020
Emit events for active connection close and fix disconnect().
8b72049 
The `Network` does currently not emit events for actively
closed connections, e.g. via `EstablishedConnection::close`
or `ConnectedPeer::disconnect()`. As a result, when actively
closing connections, there will be `ConnectionEstablished`
events emitted without eventually a matching `ConnectionClosed`
event. This seems undesirable and has the consequence that
the `Swarm::ban_peer_id` feature in `libp2p-swarm` does not
result in appropriate calls to `NetworkBehaviour::inject_connection_closed`
and `NetworkBehaviour::inject_disconnected`. Furthermore,
the `disconnect()` functionality in `libp2p-core` is currently
broken as it leaves the `Pool` in an inconsistent state.

This commit does the following:

  1. When connection background tasks are dropped
     (i.e. removed from the `Manager`), they
     always terminate immediately, without attempting
     an orderly close of the connection.
  2. An orderly close is sent to the background task
     of a connection as a regular command. The
     background task emits a `Closed` event
     before terminating.
  3. `Pool::disconnect()` removes all connection
     tasks for the affected peer from the `Manager`,
     i.e. without an orderly close, thereby also
     fixing the discovered state inconsistency
     due to not removing the corresponding entries
     in the `Pool` itself after removing them from
     the `Manager`.
  4. A new test is added to `libp2p-swarm` that
     exercises the ban/unban functionality and
     places assertions on the number and order
     of calls to the `NetworkBehaviour`. In that
     context some new testing utilities have
     been added to `libp2p-swarm`.

This addresses libp2p#1584.
@romanb romanb mentioned this issue Jun 19, 2020
Merged
romanb added a commit that referenced this issue Aug 4, 2020
@romanb @twittner
[core/swarm] Emit events for active connection close and fix `disconn…
8e1d4ed 
…ect()`. (#1619)

* Emit events for active connection close and fix `disconnect()`.

The `Network` does currently not emit events for actively
closed connections, e.g. via `EstablishedConnection::close`
or `ConnectedPeer::disconnect()`. As a result, when actively
closing connections, there will be `ConnectionEstablished`
events emitted without eventually a matching `ConnectionClosed`
event. This seems undesirable and has the consequence that
the `Swarm::ban_peer_id` feature in `libp2p-swarm` does not
result in appropriate calls to `NetworkBehaviour::inject_connection_closed`
and `NetworkBehaviour::inject_disconnected`. Furthermore,
the `disconnect()` functionality in `libp2p-core` is currently
broken as it leaves the `Pool` in an inconsistent state.

This commit does the following:

  1. When connection background tasks are dropped
     (i.e. removed from the `Manager`), they
     always terminate immediately, without attempting
     an orderly close of the connection.
  2. An orderly close is sent to the background task
     of a connection as a regular command. The
     background task emits a `Closed` event
     before terminating.
  3. `Pool::disconnect()` removes all connection
     tasks for the affected peer from the `Manager`,
     i.e. without an orderly close, thereby also
     fixing the discovered state inconsistency
     due to not removing the corresponding entries
     in the `Pool` itself after removing them from
     the `Manager`.
  4. A new test is added to `libp2p-swarm` that
     exercises the ban/unban functionality and
     places assertions on the number and order
     of calls to the `NetworkBehaviour`. In that
     context some new testing utilities have
     been added to `libp2p-swarm`.

This addresses #1584.

* Update swarm/src/lib.rs

Co-authored-by: Toralf Wittner <tw@dtex.org>

* Incorporate some review feedback.

* Adapt to changes in master.

* More verbose panic messages.

* Simplify

There is no need for a `StartClose` future.

* Fix doc links.

* Further small cleanup.

* Update CHANGELOGs and versions.

Co-authored-by: Toralf Wittner <tw@dtex.org>
@romanb
Copy link
Contributor

romanb commented Aug 4, 2020

This should be fixed in master and thus in the next release.

@romanb romanb closed this as completed Aug 4, 2020
santos227 pushed a commit to santos227/rustlib that referenced this issue Jun 20, 2022
@romanb @twittner
[core/swarm] Emit events for active connection close and fix `disconn…
474d2c0 
…ect()`. (#1619)

* Emit events for active connection close and fix `disconnect()`.

The `Network` does currently not emit events for actively
closed connections, e.g. via `EstablishedConnection::close`
or `ConnectedPeer::disconnect()`. As a result, when actively
closing connections, there will be `ConnectionEstablished`
events emitted without eventually a matching `ConnectionClosed`
event. This seems undesirable and has the consequence that
the `Swarm::ban_peer_id` feature in `libp2p-swarm` does not
result in appropriate calls to `NetworkBehaviour::inject_connection_closed`
and `NetworkBehaviour::inject_disconnected`. Furthermore,
the `disconnect()` functionality in `libp2p-core` is currently
broken as it leaves the `Pool` in an inconsistent state.

This commit does the following:

  1. When connection background tasks are dropped
     (i.e. removed from the `Manager`), they
     always terminate immediately, without attempting
     an orderly close of the connection.
  2. An orderly close is sent to the background task
     of a connection as a regular command. The
     background task emits a `Closed` event
     before terminating.
  3. `Pool::disconnect()` removes all connection
     tasks for the affected peer from the `Manager`,
     i.e. without an orderly close, thereby also
     fixing the discovered state inconsistency
     due to not removing the corresponding entries
     in the `Pool` itself after removing them from
     the `Manager`.
  4. A new test is added to `libp2p-swarm` that
     exercises the ban/unban functionality and
     places assertions on the number and order
     of calls to the `NetworkBehaviour`. In that
     context some new testing utilities have
     been added to `libp2p-swarm`.

This addresses libp2p/rust-libp2p#1584.

* Update swarm/src/lib.rs

Co-authored-by: Toralf Wittner <tw@dtex.org>

* Incorporate some review feedback.

* Adapt to changes in master.

* More verbose panic messages.

* Simplify

There is no need for a `StartClose` future.

* Fix doc links.

* Further small cleanup.

* Update CHANGELOGs and versions.

Co-authored-by: Toralf Wittner <tw@dtex.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@romanb @koivunej