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

swarm/behaviour: Inject events via single method #2832

Closed
mxinden opened this issue Aug 20, 2022 · 6 comments · Fixed by #3011
Closed

swarm/behaviour: Inject events via single method #2832

mxinden opened this issue Aug 20, 2022 · 6 comments · Fixed by #3011

Comments

@mxinden
Copy link
Member

mxinden commented Aug 20, 2022

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 swarm/: NetworkBehaviour/ConnectionHandler cross-communication #2680 and swarm/: Support generic connection management through NetworkBehaviour #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.

@mxinden mxinden changed the title swarm/behaviour: Inject events via NetworkBehaviour::onEvent(&mut self, event: InEvent) swarm/behaviour: Inject events via single method Aug 20, 2022
@thomaseizinger
Copy link
Contributor

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
Copy link
Contributor

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

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Aug 20, 2022

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
Copy link
Member

👍 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
Copy link
Member Author

mxinden commented Sep 4, 2022

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
Copy link
Member Author

mxinden commented Sep 4, 2022

Cross-referencing implementation of this proposal #2867.

mxinden added a commit to mxinden/rust-libp2p that referenced this issue Oct 11, 2022
This is not interesting in itself for stakeholders. Instead it will be tracked with the cross
behaviour communication. See also libp2p#2832.
@mergify mergify bot closed this as completed in #3011 Nov 17, 2022
mergify bot pushed a commit that referenced this issue Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants