swarm/behaviour: Inject events via single method

[mxinden]

Description

Replace the various NetworkBehaviour::inject_xxx methods with a single NetworkBehaviour::on_event method and a InEvent enum.

Example replacing NetworkBehaviour::inject_connection_established:

diff --git a/swarm/src/behaviour.rs b/swarm/src/behaviour.rs
index c0ac5976..c10adb7d 100644
--- a/swarm/src/behaviour.rs
+++ b/swarm/src/behaviour.rs
@@ -191,7 +191,13 @@ pub trait NetworkBehaviour: 'static {
         vec![]
     }
 
+    fn on_event(&mut self, _event: InEvent) {}
+
     /// Informs the behaviour about a newly established connection to a peer.
+    #[deprecated(
+        since = "0.39.0",
+        note = "Handle `InEvent::ConnectionEstablished` in `NetworkBehaviour::on_event` instead."
+    )]
     fn inject_connection_established(
         &mut self,
         _peer_id: &PeerId,
@@ -778,3 +784,15 @@ impl Default for CloseConnection {
         CloseConnection::All
     }
 }
+
+pub enum InEvent<'a> {
+    /// Informs the behaviour about a newly established connection to a peer.
+    ConnectionEstablished {
+        peer_id: PeerId,
+        connection_id: ConnectionId,
+        endpoint: &'a ConnectedPoint,
+        // TODO: Would a slice not be better?
+        failed_addresses: Option<&'a Vec<Multiaddr>>,
+        other_established: usize,
+    },
+}
diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
index 38df855c..dfefb050 100644
--- a/swarm/src/lib.rs
+++ b/swarm/src/lib.rs
@@ -710,6 +710,14 @@ where
                         failed_addresses.as_ref(),
                         non_banned_established,
                     );
+                    self.behaviour
+                        .on_event(behaviour::InEvent::ConnectionEstablished {
+                            peer_id,
+                            connection_id: id,
+                            endpoint: &endpoint,
+                            failed_addresses: failed_addresses.as_ref(),
+                            other_established: non_banned_established,
+                        });
                     return Some(SwarmEvent::ConnectionEstablished {
                         peer_id,
                         num_established,

Motivation

• Decision whether to ignore an event (e.g. the event of a new connection being established) is made at the NetworkBehaviour trait definition by providing a default implementation or not. I argue this decision should be made by the user not by libp2p, i.e. I think users should decide whether to exhaustively match on all event handlers, or ignore most through a wildcard match arm. This is especially relevant when introducing new event handlers in the future.
• The amount of event handler methods is becoming too long, especially with https://github.com/libp2p/rust-libp2p/issues/2680 and https://github.com/libp2p/rust-libp2p/pull/2828/
• I find a single event handler multiplexing events through an enum more intuitive than many inject_xxx methods.
• Having a single on_event method helps differentiate the event handling code in NetworkBehaviour from the non event handling code (new_handler, addresses_of_peer and poll).

What do folks think? Would you be in favor of this change?

Requirements

Open questions

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

Yes, unless someone else would like to.

[thomaseizinger]

I agree with your concerns.

One thing to note is that the extra match will ident all code handling the event by two tabs: one for the match and one for the match arm.

To keep things readable, users will likely extract their own functions anyway except for really simple cases which brings us back to square one with more boilerplate except that the users is more in control of which events to handle.

Another problem is that users will likely go unnoticed of new events that they should handle.

Another option would be if a NetworkBehaviour could "subscribe" to individual events but I am not sure how to model that in a type safe way.

Unfinished thought:

We could have another macro that lets users tag functions as event handlers, thus picking the ones they want to handle and the macro would fill in the remaining ones:

#[libp2p::polyfill_unhandled_events]
impl MyBehaviour {
    // Macro can detect this by convention or we add a `#[handler]` attribute
    fn on_connection_established(&mut self, event: OnConnectionEstablished) -> {}
}

This has big downsides too though:

• No typesystem guidance for the user on what is available.
• Overall harder to understand because macros.

[thomaseizinger]

If we go with an enum, we could probably also unify SwarmEvent and the above InEvent.

[thomaseizinger]

Thinking about it more, a single on_event function seems like the right primitive to me. Everything else, including the macro above, can be built on top of that.

[dignifiedquire]

👍 in general from my side, I am always worried if I am handling all events that I need to, and this would make it a lot easier to guard against the issue.

[mxinden]

Related to the change on NetworkBehaviour, I would like to to do the corresponding change to ConnectionHandler as well, namely to replace its inject_* methods with a single on_event method. I think this does not have to happen within the same pull request, though preferably for the sake of consistency, within the same release.

[mxinden]

Cross-referencing implementation of this proposal https://github.com/libp2p/rust-libp2p/pull/2867.