Deprecate NetworkBehaviour listener callback methods

[jxs]

Summary

As per the discussion started on #3011 (comment):

• A NetworkBehaviour has no control over the listeners. Why should it be interested in knowing that there is a new one etc?
• What is it going to do with the error of a failed listener? These errors aren't fatal, the listener will keep going afterwards.
• A search around GitHub shows that all usages found are either delegating to inner implementations, in test files or empty.

Most of the callbacks were added on #2011 to be able to be used by ipfs-embed but according to @thomaseizinger:

• Metrics for general stuff like listeners should probably be collected outside of the behaviour to make sure it happens in a
  canonical place and not in multiple places. What makes the Peers behaviour privileged to collect stats on the number of listeners compared to any other behaviour?
• Logging could happen outside too and would probably lead to less duplication: What if every behaviour starts to log these? If we want these logs, having them here would probably be better.
• What is the value of wrapping and returning the event? The information is already available to the caller by means of polling the swarm itself. Exposing it again through a stream seems like duplicated work.

Proposed Solution

Remove the callbacks and thus get of the problem of errors that can only be passed around by reference. Callbacks:
inject_new_listener
inject_new_listen_addr
inject_expired_listen_addr
inject_expired_external_addr
inject_listener_error
inject_listener_closed

these callbacks will be renamed to on_ after #3011 lands, see #2832 for more info on it

Are you planning to do it yourself in a pull request?

Maybe.

@thomaseizinger ptal and feel free to edit any mistake I made.

CC @rkuhn @dvc94ch

[thomaseizinger]

Remove the callbacks and thus get of the problem of errors that can only be passed around by reference. Callbacks: inject_new_listener inject_new_listen_addr inject_expired_listen_addr inject_expired_external_addr inject_listener_error inject_listener_closed

I think inject_new_listen_addr, inject_expired_listen_addr and inject_expired_external_addr are worthwhile keeping but I'd probably remove the ListenerId from them because it is an identifier that doesn't mean anything to a behaviour.

[dvc94ch]

libp2p has certainly changed a lot since I made the PR adding those. However even with your changes it's not clear why there should be a FromSwarm and a SwarmEvent enum with different information. I'd bring it to it's logical conclusion of removing SwarmEvent entirely. [0][1]

What is the value of wrapping and returning the event? The information is already available to the caller by means of polling the swarm itself. Exposing it again through a stream seems like duplicated work.

First of it doesn't require a &mut Swarm. Consumers of ipfs-embed don't have access to the swarm or network behaviour but still use these events.

• [0]

  rust-libp2p/swarm/src/behaviour.rs

  Line 855 in 2166857

  pub enum FromSwarm<'a, Handler: IntoConnectionHandler> {

• [1]

  rust-libp2p/swarm/src/lib.rs

  Line 139 in 2166857

  pub enum SwarmEvent<TBehaviourOutEvent, THandlerErr> {

[thomaseizinger]

What is the value of wrapping and returning the event? The information is already available to the caller by means of polling the swarm itself. Exposing it again through a stream seems like duplicated work.

First of it doesn't require a &mut Swarm. Consumers of ipfs-embed don't have access to the swarm or network behaviour but still use these events.

In that case, that abstraction should be built on top of whatever polls the swarm.

libp2p has certainly changed a lot since I made the PR adding those. However even with your changes it's not clear why there should be a FromSwarm and a SwarmEvent enum with different information. I'd bring it to it's logical conclusion of removing SwarmEvent entirely. [0][1]

They don't represent the same set of events. FromSwarm is for communicaton from the swarm to the behaviour. SwarmEvent is emitted by the swarm. If anything, we should be striving for them to be complimentary!

[dvc94ch]

well, it's annoying to have to piece together information from different sources. They overlap a lot in functionality. I'd suggest making them equivalent. Then there is stuff in NetworkBehaviourAction's that can't be done without calling a method on the swarm. Why can't I ban a peer from a network behaviour? Seems like an arbitrary restriction.

• [0] https://docs.rs/libp2p/latest/libp2p/swarm/enum.NetworkBehaviourAction.html

[dvc94ch]

usually what ends up happening when using libp2p is this:

• you spawn the swarm in a task
• you add a ctrl channel that has some Command enum with a bunch of oneshot or mpsc channels for sending back the response
• it makes little difference if you pass that channel to a network behaviour on setup or the task that polls the swarm
• the annoying thing is when you can't get everything you need in the same place

[thomaseizinger]

well, it's annoying to have to piece together information from different sources. They overlap a lot in functionality. I'd suggest making them equivalent. Then there is stuff in NetworkBehaviourAction's that can't be done without calling a method on the swarm. Why can't I ban a peer from a network behaviour? Seems like an arbitrary restriction.

• [0] https://docs.rs/libp2p/latest/libp2p/swarm/enum.NetworkBehaviourAction.html

That is a fair point. Pluggable connection management is tracked here: #2824

Extending this with bans is definitely worth considering.

the annoying thing is when you can't get everything you need in the same place

In my experience, things tend to come out cleaner when you try and implement as much networking logic in a NetworkBehaviour instead of calling various control knobs from the outside.

However, this is only really possible if the network protocols are designed to operate somewhat autonomously.

[dvc94ch]

In my experience, things tend to come out cleaner when you try and implement as much networking logic in a NetworkBehaviour instead of calling various control knobs from the outside.

You seem to be agreeing with me that reducing the reliance on swarm events seems to be a good idea?

Is there a reason we can't remove swarm events and all the swarm methods and just make the FromSwarm and NetworkBehaviourAction handle these cases?

[thomaseizinger]

In my experience, things tend to come out cleaner when you try and implement as much networking logic in a NetworkBehaviour instead of calling various control knobs from the outside.

You seem to be agreeing with me that reducing the reliance on swarm events seems to be a good idea?

Is there a reason we can't remove swarm events and all the swarm methods and just make the FromSwarm and NetworkBehaviourAction handle these cases?

Eventually we might get there yes.

One situation I don't know how to resolve is managing listen addresses. NetworkBehaviours don't know about the transport stack that is underneath them. I think that is a good thing. It allows us to test NetworkBehaviours on top of the memory transport for example.

The component that constructs the Swarm also constructs that Transport so it is in a much better position to issue listen_on calls and manage the lifecycle of listeners in general.

In general though, I do agree with you. I see Swarm as a kind of "runtime" and NetworkBehaviours as plugins where ideally, everything is a plugin.

[thomaseizinger]

NetworkBehaviours don't know about the transport stack that is underneath them. I think that is a good thing. It allows us to test NetworkBehaviours on top of the memory transport for example.

This argument isn't any good actually. A NetworkBehaviour can already dial other peers without know what transport stack it has underneath so the dial may fail. I don't see why listening is necessarily any different?

If we all NetworkBehaviours to also instruct the Swarm to listen on certain interfaces, we could eventually get rid of all SwarmEvents and only emit the behaviour events as part of polling it.

[thomaseizinger]

We are actually going to go the other way, see #3291.