swarm/behaviour: Replace inject_* with on_event #2867

mxinden · 2022-09-04T03:56:55Z

Description

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

I want to open this for early feedback before I update the many NetworkBehaviour implementations in /protocols.

Links to any relevant issues

Fixes #2832.

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

Replace the various `NetworkBehaviour::inject_*` methods with a single `NetworkBehaviour::on_event` method and a `InEvent` `enum`.

mxinden · 2022-09-04T03:58:29Z

swarm/src/behaviour/either.rs

+    fn on_event(&mut self, event: super::InEvent<Self::ConnectionHandler>) {
        match self {
-            Either::Left(a) => a.inject_listener_closed(id, reason),
-            Either::Right(b) => b.inject_listener_closed(id, reason),
+            Either::Left(b) => b.on_event(event.map_handler(
+                |h| h.unwrap_left(),
+                |h| match h {
+                    Either::Left(h) => h,
+                    Either::Right(_) => unreachable!(),
+                },
+                |e| e.unwrap_left(),
+            )),
+            Either::Right(b) => b.on_event(event.map_handler(
+                |h| h.unwrap_right(),
+                |h| match h {
+                    Either::Right(h) => h,
+                    Either::Left(_) => unreachable!(),
+                },
+                |e| e.unwrap_right(),
+            )),


While not the actual goal of this effort, this is a nice simplification for the Either and Toggle NetworkBehaviour.

In general, I expect this pull request to simplify many of these general purpose NetworkBehaviour implementations. (Not that we have that many of them.)

MarcoPolo · 2022-09-04T04:32:58Z

Exciting!

thomaseizinger

Did a first pass, looks good overall!

Had an idea how we could deprecate more functions.

swarm/src/behaviour.rs

thomaseizinger · 2022-09-04T09:23:25Z

swarm/src/handler/either.rs

+// Taken from https://github.com/bluss/either.
+impl<L, R> IntoEitherHandler<L, R> {


Do we still need IntoEitherHandler?

How else would the IntoConnectionHandler::into_handler methods of the left and right branch be called?

What I was getting at is: Can we replace it with Either?

I think we discussed this in the past, see #2192 (comment) @thomaseizinger.

You've got much better memory than I do :D

What do you think about adding a comment about it?

thomaseizinger · 2022-09-04T09:26:07Z

swarm/src/lib.rs

+            InEvent::ConnectionEstablished {
+                peer_id: _,
+                connection_id: _,
+                endpoint: _,
+                failed_addresses: _,
+                other_established: _,
+            } => {}


Why match on all of these in here?

To have a compile time error when any of them change, forcing us to revisit each implementation. I guess it is a bit overkill for this one. Let me know if you prefer this to be changed.

I think it is a good idea but I'd prefer if we do that in a more prominent place. For example, we could have a dedicated public_api.rs in swarm/tests that concerns itself with such things. I find it a bit odd that this particular impl should take care of that.

Thoughts?

swarm/src/behaviour/either.rs

…ehaviour-on-event

mxinden · 2022-09-07T06:28:30Z

Thanks @thomaseizinger for the helpful review. Mind taking another look?

mxinden · 2022-09-07T06:29:45Z

Once everyone is happy with the changes in libp2p-swarm I will start porting the many users of libp2p-swarm in /protocols to the new NetworkBehaviour::on_event methods. I plan to do so in this pull request, unless folks prefer separate pull request(s).

swarm/src/behaviour.rs

thomaseizinger · 2022-09-08T08:29:29Z

Once everyone is happy with the changes in libp2p-swarm I will start porting the many users of libp2p-swarm in /protocols to the new NetworkBehaviour::on_event methods. I plan to do so in this pull request, unless folks prefer separate pull request(s).

We would have to litter the rest of the codebase with allow(deprecated) attributes to make CI pass. I think porting the protocols in this PR is better.

…ehaviour-on-event

This allows rolling out the whole patch set in a non-breaking way.

mxinden · 2022-09-17T16:00:09Z

63b70fe provides default implementations for NetworkBehaviour::on_swarm_event and NetworkBehaviour::on_connection_handler_event, thus this patch set can be released as a non-breaking change.

Once we remove the deprecated NetworkBehaviour::inject_* methods, I think we should remove the default implementations on NetworkBehaviour::on_swarm_event and NetworkBehaviour::on_connection_handler_event as well.

thomaseizinger · 2022-09-19T05:27:07Z

63b70fe provides default implementations for NetworkBehaviour::on_swarm_event and NetworkBehaviour::on_connection_handler_event, thus this patch set can be released as a non-breaking change.

Once we remove the deprecated NetworkBehaviour::inject_* methods, I think we should remove the default implementations on NetworkBehaviour::on_swarm_event and NetworkBehaviour::on_connection_handler_event as well.

Agreed!

thomaseizinger

It is nice that we can release this as a non-breaking change.

Couple of questions:

https://github.com/libp2p/rust-libp2p/pull/2867/files#r965651079 is not yet resolved
Are we converting all behaviours in this repository to the new APIs?

thomaseizinger · 2022-09-19T05:27:46Z

swarm/src/behaviour.rs

+        // TODO: Instead of the type alias THandlerOutEvent the full definition might be more
+        // intuitive for users outside of libp2p-swarm.


Yep, esp. because the type alias is not public!

thomaseizinger · 2022-09-19T05:28:12Z

swarm/src/behaviour.rs

    ) {
+        self.on_swarm_event(InEvent::ConnectionClosed {
+            peer_id: *peer_id,


Nice that we can fix the references here!

thomaseizinger · 2022-09-19T05:29:33Z

swarm/src/behaviour.rs

@@ -721,3 +850,182 @@ impl Default for CloseConnection {
        CloseConnection::All
    }
 }
+
+// TODO: Rename to FromSwarm


Should we pool the rename together with the release of this change as well?

We could also name it FromTransport maybe? I am beginning to more and more take on the notion of libp2p-swarm being a kind of "runtime" that actually doesn't do anything by itself but passes data between other components so in a way, the swarm itself would never produce an event.

In favor of naming it FromSwarm. FromTransport imo could be confusing because its not an event emitted by a transport implementation (e.g. in the relay protocol we do have events that are actually produced by the relay client transport). Also, some events are still caused by the swarm and not the transport, e.g. a DialFailure because of swarm connection limits.
That being said, I do generally like the conception of swarm being some kind of "runtime".

thomaseizinger · 2022-09-19T05:34:30Z

swarm/src/handler/either.rs

+// Taken from https://github.com/bluss/either.
+impl<L, R> IntoEitherHandler<L, R> {


What do you think about adding a comment about it?

mxinden · 2022-10-12T12:38:00Z

Closing here in favor of #3011.

swarm/behaviour: Replace inject_* with on_event

90a1349

Replace the various `NetworkBehaviour::inject_*` methods with a single `NetworkBehaviour::on_event` method and a `InEvent` `enum`.

mxinden commented Sep 4, 2022

View reviewed changes

mxinden mentioned this pull request Sep 4, 2022

swarm/behaviour: Inject events via single method #2832

Closed

thomaseizinger reviewed Sep 4, 2022

View reviewed changes

mxinden mentioned this pull request Sep 5, 2022

swarm/handler: Rename InEvent and OutEvent to FromBehaviour and ToBehaviour #2854

Closed

mxinden added 3 commits September 7, 2022 15:11

swarm/behaviour: Delegate to on_event from inject_*

de5c46d

swarm/behaviour: Address code review

021a7f1

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into b…

ff08ccd

…ehaviour-on-event

mxinden marked this pull request as ready for review September 7, 2022 06:28

mxinden requested a review from thomaseizinger September 7, 2022 06:28

elenaf9 reviewed Sep 7, 2022

View reviewed changes

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Sep 8, 2022

View reviewed changes

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

swarm/src/behaviour.rs Outdated Show resolved Hide resolved

mxinden added 5 commits September 17, 2022 17:09

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into b…

3d8561d

…ehaviour-on-event

swarm/behaviour: Add on_connection_handler_event

07a4f20

swarm/behaviour: Use slice

99a4b64

swarm/behaviour: Rename to maybe_map_handler

46f20fa

swarm/behaviour: Provide default for on_swarm and on_connection

63b70fe

This allows rolling out the whole patch set in a non-breaking way.

thomaseizinger reviewed Sep 19, 2022

View reviewed changes

jxs mentioned this pull request Oct 11, 2022

swarm/behaviour: Replace inject_* with on_event #3011

Merged

4 tasks

mxinden closed this Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm/behaviour: Replace inject_* with on_event #2867

swarm/behaviour: Replace inject_* with on_event #2867

mxinden commented Sep 4, 2022

mxinden Sep 4, 2022

MarcoPolo commented Sep 4, 2022

thomaseizinger left a comment

thomaseizinger Sep 4, 2022

mxinden Sep 7, 2022

thomaseizinger Sep 8, 2022

mxinden Sep 17, 2022

thomaseizinger Sep 19, 2022

thomaseizinger Sep 19, 2022

thomaseizinger Sep 4, 2022

mxinden Sep 7, 2022

thomaseizinger Sep 8, 2022

mxinden commented Sep 7, 2022

mxinden commented Sep 7, 2022

thomaseizinger commented Sep 8, 2022 •

edited

Loading

mxinden commented Sep 17, 2022

thomaseizinger commented Sep 19, 2022

thomaseizinger left a comment

thomaseizinger Sep 19, 2022

thomaseizinger Sep 19, 2022

thomaseizinger Sep 19, 2022

thomaseizinger Sep 19, 2022

elenaf9 Sep 19, 2022

thomaseizinger Sep 19, 2022

mxinden commented Oct 12, 2022

		// Taken from https://github.com/bluss/either.
		impl<L, R> IntoEitherHandler<L, R> {

		// TODO: Instead of the type alias THandlerOutEvent the full definition might be more
		// intuitive for users outside of libp2p-swarm.

swarm/behaviour: Replace inject_* with on_event #2867

swarm/behaviour: Replace inject_* with on_event #2867

Conversation

mxinden commented Sep 4, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

Choose a reason for hiding this comment

MarcoPolo commented Sep 4, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Sep 7, 2022

mxinden commented Sep 7, 2022

thomaseizinger commented Sep 8, 2022 • edited Loading

mxinden commented Sep 17, 2022

thomaseizinger commented Sep 19, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Oct 12, 2022

thomaseizinger commented Sep 8, 2022 •

edited

Loading