swarm/handler: replace inject_* methods #3085

jxs · 2022-11-04T13:57:50Z

Description

Previously, we had one callback for each kind of message that a ConnectionHandler would receive from either its NetworkBehaviour or the connection itself.

With this patch, we combine these functions, resulting in two callbacks:

on_behaviour_event
on_connection_event

Resolves #3080.

Links to any relevant issues

ConnectionHandler counterpart to #3011

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

on ConnectionHandler implementations

mxinden

Cool to see this work happening. Let us know once this is ready for a review.

mxinden · 2022-11-08T18:30:16Z

swarm/src/handler.rs

+    #[deprecated(
+        since = "0.41.0",
+        note = "Handle `StreamEvent::FullyNegotiatedInbound` on `ConnectionHandler::on_event` instead.
+        the default of implemention of this inject_*` method delegates to it."


Suggested change

the default of implemention of this inject_*` method delegates to it."

The default implementation of this `inject_*` method delegates to it."

Thanks Max! addressed.

Cool to see this work happening. Let us know once this is ready for a review.

Thanks Max, yeah I am waiting for #3011 to be merged cause this PR also touches some of the same files to then rebase against master and mark it as ready.

mxinden · 2022-11-08T18:35:03Z

swarm/src/handler.rs

+
+/// Enumeration with the list of the possible stream events
+/// to pass to [`on_event`](ConnectionHandler::on_event).
+pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {


An alternative to explore (not saying it is a good idea) would be to take a single trait parameter, namely ConnectionHandler here.

Suggested change

pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

pub enum StreamEvent<'a, Handler: ConnectionHandler> {

Thanks for bringing this Max,
yeah it works but we need to relax the sizedness to Handler: ConnectionHandler + ?Sized because on_event's argument event: StreamEvent<Self> which afaik will probably be problematic if we in the future define methods on ConnectionHandler that take self and StreamEvent<Self> cause Self must be Sized and StreamEvent<Self> may be ?Sized. Would you prefer that over the multiple generic parameters?

yeah it works but we need to relax the sizedness to Handler: ConnectionHandler + ?Sized because on_event's argument event: StreamEvent<Self> which afaik will probably be problematic if we in the future define methods on ConnectionHandler that take self and StreamEvent<Self> cause Self must be Sized and StreamEvent<Self> may be ?Sized.

I am not sure I am following. In which case would ConnectionHandler not be Sized? In which case would this be problematic?

The approach you suggest on #3085 (comment):

Suggested change

pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

implies relaxing StreamEvent<Handler> to StreamEvent<Handler: ConnectionHandler + ?Sized>.

thomaseizinger

Looking good! Thank you :)

thomaseizinger · 2022-11-07T23:24:01Z

swarm/src/handler.rs

+    /// informs the handler about an event coming from the outside in the handler.
+    fn on_behaviour_event(&mut self, _event: Self::InEvent) {}
+
+    fn on_event(


Should this be an on_connection_event maybe? It would be nicer to have both on_ handlers use some kind of qualifier.

yeah makes sense, thanks Thomas updated! :)

thomaseizinger · 2022-11-07T23:24:56Z

swarm/src/handler/map_in.rs

+        match event {
+            StreamEvent::FullyNegotiatedInbound(FullyNegotiatedInbound { protocol, info }) =>
+            {
+                #[allow(deprecated)]
+                self.inner.inject_fully_negotiated_inbound(protocol, info)
+            }
+            StreamEvent::FullyNegotiatedOutbound(FullyNegotiatedOutbound { protocol, info }) =>
+            {
+                #[allow(deprecated)]
+                self.inner.inject_fully_negotiated_outbound(protocol, info)
+            }
+            StreamEvent::AddressChange(AddressChange { new_address }) =>
+            {
+                #[allow(deprecated)]
+                self.inner.inject_address_change(new_address)
+            }
+            StreamEvent::DialUpgradeError(DialUpgradeError { info, error }) =>
+            {
+                #[allow(deprecated)]
+                self.inner.inject_dial_upgrade_error(info, error)
+            }
+            StreamEvent::ListenUpgradeError(ListenUpgradeError { info, error }) =>
+            {
+                #[allow(deprecated)]
+                self.inner.inject_listen_upgrade_error(info, error)
+            }
+        }


I guess this can be replaced with self.inner.on_event once we remove them right?

yeah exactly, also see #3011 (comment)

thomaseizinger · 2022-11-07T23:25:26Z

swarm/src/handler/pending.rs

 use std::task::{Context, Poll};
 use void::Void;

+use super::{FullyNegotiatedInbound, FullyNegotiatedOutbound, StreamEvent};


I'd prefer to not use super imports.

yeah, makes total sense, specailly cause we are already importing with crate::handler. Thanks Thomas, updated!

thomaseizinger · 2022-11-07T23:29:24Z

swarm/src/handler.rs

+
+/// Enumeration with the list of the possible stream events
+/// to pass to [`on_event`](ConnectionHandler::on_event).
+pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {


Suggested change

pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

pub enum ConnectionEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

…n-handler-inject-on

and StreamEvent -> ConnectionEvent.

mxinden

Minor comments. Otherwise this looks good to me.

mxinden · 2022-11-15T10:30:53Z

swarm/src/handler.rs

+    #[deprecated(
+        since = "0.41.0",
+        note = "Handle `ConnectionEvent::FullyNegotiatedInbound` on `ConnectionHandler::on_connection_event` instead.
+        The default of implemention of this `inject_*` method delegates to it."


Suggested change

The default of implemention of this `inject_*` method delegates to it."

The default implementation of this `inject_*` method delegates to it."

Thanks Max 🙇

mxinden · 2022-11-15T10:32:06Z

swarm/src/handler.rs

        ConnectionHandlerSelect::new(self, other)
    }
+
+    /// informs the handler about an event coming from the outside in the handler.


Suggested change

/// informs the handler about an event coming from the outside in the handler.

/// Informs the handler about an event from the [`NetworkBehaviour`].

I would find this easier to understand. What do you think?

yeah I agree, thanks!

mxinden · 2022-11-15T10:33:15Z

swarm/src/handler.rs

+pub enum ConnectionEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {
+    /// Informs the handler about the output of a successful upgrade on a new inbound substream.
+    FullyNegotiatedInbound(FullyNegotiatedInbound<IP, IOI>),
+    /// Informs the handler about successful upgrade on a new outbound stream.


Suggested change

/// Informs the handler about successful upgrade on a new outbound stream.

/// Informs the handler about the output of a successful upgrade on a new outbound stream.

To be consistent with the above.

yeah makes sense, thanks Max!

…n-handler-inject-on

thomaseizinger · 2022-11-15T22:49:41Z

swarm/src/handler.rs

+    fn on_behaviour_event(&mut self, _event: Self::InEvent) {}
+
+    fn on_connection_event(
+        &mut self,
+        _event: ConnectionEvent<
+            Self::InboundProtocol,
+            Self::OutboundProtocol,
+            Self::InboundOpenInfo,
+            Self::OutboundOpenInfo,
+        >,
+    ) {
+    }
+}


I wonder, once Self::InEvent is called Self::FromBehaviour, should we rename ConnectionEvent to FromConnection?

Works for me, though I suggest not doing that within this pull request.

mxinden

Thanks for the follow-ups.

mxinden · 2022-11-16T12:27:08Z

Happy to merge here. Let's give @thomaseizinger antoher chance to review.

thomaseizinger

LGTM!

mxinden · 2022-11-17T09:06:07Z

@jxs can you take a look at the CI failure?

 error: unresolved link to `NetworkBehaviour`
   --> swarm/src/handler.rs:281:55
    |
281 |     /// Informs the handler about an event from the [`NetworkBehaviour`].
    |                                                       ^^^^^^^^^^^^^^^^ no item named `NetworkBehaviour` in scope
    |
    = note: requested on the command line with `-D rustdoc::broken-intra-doc-links`
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

error: unresolved link to `ConnectionHandler::on_event`
   --> swarm/src/handler.rs:297:29
    |
297 | /// to pass to [`on_event`](ConnectionHandler::on_event).
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ConnectionHandler` has no associated item named `on_event`

error: could not document `libp2p-swarm`

https://github.com/libp2p/rust-libp2p/actions/runs/3474448909/jobs/5807623337

mergify · 2022-11-17T09:29:34Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

…n-handler-inject-on

mxinden · 2022-11-17T11:45:15Z

Just noticed, this is missing changelog entries, right? At least for libp2p-swarm we should have an entry describing the deprecation path. I suggest adding changelog entries to this pull request similar to the ones in #3011.

Can you tackle this @jxs?

…n-handler-inject-on

jxs · 2022-11-17T14:27:12Z

Just noticed, this is missing changelog entries, right? At least for libp2p-swarm we should have an entry describing the deprecation path. I suggest adding changelog entries to this pull request similar to the ones in #3011.

Can you tackle this @jxs?

thanks for noticing Max! Yeah added the missing CHANGELOG.md files to the affected crates.

mxinden

🚀

jxs marked this pull request as draft November 4, 2022 13:57

jxs mentioned this pull request Nov 4, 2022

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

Closed

jxs force-pushed the connection-handler-inject-on branch 9 times, most recently from d926cd2 to d60c0e7 Compare November 8, 2022 17:21

jxs added 8 commits November 8, 2022 17:37

swarm/handler: replace inject_* methods

5f95709

dcutr: replace inject_* with on_event,

758ee6f

on ConnectionHandler implementations

gossipsub: replace inject_* with on_event

48ed9d6

identify: replace inject_* with on_event

9b44019

kad: replace inject_* with on_event

5c50aca

ping: replace inject_* with on_event

d78b545

relay: replace inject_* with on_event

6767d91

request-response: replace inject_* with on_event

cf9768f

jxs force-pushed the connection-handler-inject-on branch from d60c0e7 to cf9768f Compare November 8, 2022 17:37

mxinden reviewed Nov 8, 2022

View reviewed changes

review: address typos

14ff311

thomaseizinger mentioned this pull request Nov 11, 2022

swarm: Split the ConnectionHandler::inject_listen_upgrade_error into two methods #3098

Closed

4 tasks

thomaseizinger reviewed Nov 11, 2022

View reviewed changes

mxinden mentioned this pull request Nov 12, 2022

swarm/behaviour: Replace inject_* with on_event #3011

Merged

4 tasks

jxs added 3 commits November 14, 2022 12:31

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

0b866a1

…n-handler-inject-on

review: rename on_event -> on_connection_event,

e3a7dd2

and StreamEvent -> ConnectionEvent.

review: remove super:: usages

efa29e1

jxs marked this pull request as ready for review November 14, 2022 14:00

mxinden approved these changes Nov 15, 2022

View reviewed changes

jxs added 2 commits November 15, 2022 21:54

review: fix typos

3d67cae

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

2b91c32

…n-handler-inject-on

thomaseizinger reviewed Nov 15, 2022

View reviewed changes

mxinden approved these changes Nov 16, 2022

View reviewed changes

mxinden requested a review from thomaseizinger November 16, 2022 12:25

thomaseizinger approved these changes Nov 17, 2022

View reviewed changes

jxs added 2 commits November 17, 2022 10:53

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

f02aaad

…n-handler-inject-on

review: fix doc errors on ConnectionHandler

448669d

jxs added 2 commits November 17, 2022 14:24

review: add missing CHANGELOG.md files to each crate

32119cf

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

f621eb2

…n-handler-inject-on

mxinden approved these changes Nov 17, 2022

View reviewed changes

mxinden added the send-it label Nov 17, 2022

mergify bot merged commit 7803524 into libp2p:master Nov 17, 2022

jxs mentioned this pull request Dec 21, 2022

refactor(swarm): remove deprecated inject calls #3264

Merged

4 tasks

	the default of implemention of this inject_*` method delegates to it."
	The default implementation of this `inject_*` method delegates to it."

	pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {
	pub enum StreamEvent<'a, Handler: ConnectionHandler> {

	pub enum StreamEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {
	pub enum ConnectionEvent<'a, IP: InboundUpgradeSend, OP: OutboundUpgradeSend, IOI, OOI> {

	The default of implemention of this `inject_*` method delegates to it."
	The default implementation of this `inject_*` method delegates to it."

	/// informs the handler about an event coming from the outside in the handler.
	/// Informs the handler about an event from the [`NetworkBehaviour`].

	/// Informs the handler about successful upgrade on a new outbound stream.
	/// Informs the handler about the output of a successful upgrade on a new outbound stream.

swarm/handler: replace inject_* methods #3085

swarm/handler: replace inject_* methods #3085

Uh oh!

Conversation

jxs commented Nov 4, 2022 • edited by thomaseizinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Nov 16, 2022

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Nov 17, 2022

Uh oh!

mergify bot commented Nov 17, 2022

Uh oh!

mxinden commented Nov 17, 2022

Uh oh!

jxs commented Nov 4, 2022 •

edited by thomaseizinger

Loading