refactor(swarm): remove deprecated inject calls #3264

jxs · 2022-12-20T00:18:59Z

Description

Finishes work first started with #2832

Notes

review by commit is suggested :)

Links to any relevant issues

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

to on_event

swarm-derive: remove inject methods from derive macro.

thomaseizinger

Nice, thanks for following up on that!

I think we can simplify some of this a bit. Let me know if you need more input :)

swarm/src/handler.rs

thomaseizinger · 2022-12-20T00:52:11Z

swarm/src/handler/either.rs

-                    #[allow(deprecated)]
-                    handler.inject_fully_negotiated_outbound(output, info)
+                (Either::Left(handler), EitherOutput::First(output), Either::Left(info)) => {
+                    handler.on_connection_event(ConnectionEvent::FullyNegotiatedOutbound(


It seems weird that we need to match on the event here (line 174). I think it would be better if we added a

impl ConnectionEvent<EitherOutput, EitherOutput, Either, Either> { }

block where we map the entire event into a Either<ConnectionEvent, ConnectionEvent> so you can directly dispatch the event to the left or the right handler.

yeah, that would be great. I tried:

impl<'a, R, L> ConnectionEvent< 'a, EitherUpgrade<SendWrapper<L::InboundProtocol>, SendWrapper<R::InboundProtocol>>, EitherUpgrade<SendWrapper<L::OutboundProtocol>, SendWrapper<R::OutboundProtocol>>, Either<L::InboundOpenInfo, R::InboundOpenInfo>, Either<L::OutboundOpenInfo, R::OutboundOpenInfo>, > where L: ConnectionHandler, R: ConnectionHandler, {}

but get that L and R are not constrained. I wonder if #3085 (comment) would have changed that.

I was thinking something like e3480d2. It doesn't compile yet but maybe you can make it work!

thanks for the lead Thomas, managed to make it work. Used Either::Left for AddressChange instead of the Result as it seemed more semantically accurate in the sense that it's not err, it just doesn't matter if it's either Left or Right for that variant, cause when matching on on_connection_event we still have the Either::Right . Ptal and see if you agree.

thanks for the lead Thomas, managed to make it work. Used Either::Left for AddressChange instead of the Result as it seemed more semantically accurate in the sense that it's not err, it just doesn't matter if it's either Left or Right for that variant, cause when matching on on_connection_event we still have the Either::Right . Ptal and see if you agree.

Yeah I wasn't sure about that one. I think one could argue it is an error because we couldn't unambiguously transpose it. Can we make it work with Result or is moving to Either::Left essential?

Alternatively, we could wrap it in another Either but that is a bit weird. If possible, I'd prefer the Result return type and some documentation on why it is necessary.

no you are right, documenting it informing that an Err can be because AddressChange isn't transposable makes sense to me, updated it :)

thomaseizinger · 2022-12-20T00:53:46Z

swarm/src/handler/select.rs

            <Self as ConnectionHandler>::OutboundProtocol,
        >,
    ) {
        match (info, error) {


Same here, I think we could make use of the same mapping function from above here.

Addressed, see if that's what you were expecting Thomas

mxinden

Thanks for tackling this.

swarm-derive/src/lib.rs

mxinden · 2022-12-21T09:41:10Z

swarm/src/behaviour.rs

-    fn on_swarm_event(&mut self, _event: FromSwarm<Self::ConnectionHandler>) {}
+    fn on_swarm_event(&mut self, event: FromSwarm<Self::ConnectionHandler>);


…ject-calls

jxs · 2022-12-20T00:21:26Z

protocols/rendezvous/src/substream_handler.rs

    }

-    fn inject_fully_negotiated_inbound(
+    fn on_connection_event(


this was missed in the previous #3085 sorry, should I add a new changelog entry CC @mxinden? (there was already one in the previous version mentioning the replacement of the methods for Handler)

I don't think this change is breaking, right? Thus advocating for no changelog.

On a related note, shouldn't our CI have caught this? Shouldn't this have generated a deprecation warning and thus shouldn't CI have failed? Can you investigate this @jxs?

yeah this isn't a breaking change afaik, as they are only called by the Behaviour

On a related note, shouldn't our CI have caught this? Shouldn't this have generated a deprecation warning and thus shouldn't CI have failed?

good point, cargo doesn't trigger deprecation warning on the implementation, only on the invocation IRC, do we have any code triggering calls to the methods?

jxs · 2022-12-21T16:55:08Z

swarm/src/handler/either.rs

-                    #[allow(deprecated)]
-                    handler.inject_fully_negotiated_outbound(output, info)
+                (Either::Left(handler), EitherOutput::First(output), Either::Left(info)) => {
+                    handler.on_connection_event(ConnectionEvent::FullyNegotiatedOutbound(


yeah, that would be great. I tried:

impl<'a, R, L> ConnectionEvent< 'a, EitherUpgrade<SendWrapper<L::InboundProtocol>, SendWrapper<R::InboundProtocol>>, EitherUpgrade<SendWrapper<L::OutboundProtocol>, SendWrapper<R::OutboundProtocol>>, Either<L::InboundOpenInfo, R::InboundOpenInfo>, Either<L::OutboundOpenInfo, R::OutboundOpenInfo>, > where L: ConnectionHandler, R: ConnectionHandler, {}

but get that L and R are not constrained. I wonder if #3085 (comment) would have changed that.

swarm-derive/src/lib.rs

swarm/src/handler.rs

mxinden

Thanks for the follow-ups.

mxinden · 2022-12-22T15:33:21Z

protocols/rendezvous/src/substream_handler.rs

    }

-    fn inject_fully_negotiated_inbound(
+    fn on_connection_event(


I don't think this change is breaking, right? Thus advocating for no changelog.

On a related note, shouldn't our CI have caught this? Shouldn't this have generated a deprecation warning and thus shouldn't CI have failed? Can you investigate this @jxs?

mxinden · 2022-12-22T15:35:30Z

swarm-derive/CHANGELOG.md

 - Replace `NetworkBehaviour` Derive macro deprecated `inject_*` method implementations
  with the new `on_swarm_event` and `on_connection_handler_event`.


This entry was previously mistakenly in v0.31.0, right? I.e. we did not actually move to the on_ methods in swarm-derive in v0.31.0 as otherwise our non-breaking-change hack would not have worked, correct?

yeah exactly, thanks for referring this Max.

mxinden

Please trigger mergify once the above discussion with @thomaseizinger is resolved.

mergify · 2022-12-22T16:46:11Z

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

for ConnectionEvent and DialUpgradeError, to help when matching on them for Either and Select.

…ject-calls

thomaseizinger · 2022-12-22T23:25:53Z

swarm/src/handler/select.rs

+    S1OOI: Send + 'static,
+    S2OOI: Send + 'static,
+{
+    fn transpose(self) -> Either<DialUpgradeError<S1OOI, S1OP>, DialUpgradeError<S2OOI, S2OP>> {


I like that you split this out. I actually think it makes sense to split this out for all structs of ConnectionEvent. Can we reuse this as part of implementing the bigger transpose function?

Ah, thanks :D but I did it because in some variants like ListenUpgradeError or AddressChange transpose doesn't win us anything as we have to call it for both Select variants. I updated the code implementing for the variants where it makes sense, ptal Thomas and see if it makes sense to you.

mergify · 2022-12-23T00:14:38Z

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

for the remaining ConnectionEvents that is useful to implement on.

to Result and document it.

thomaseizinger · 2022-12-24T05:37:07Z

swarm/src/handler/either.rs

+            ConnectionEvent::FullyNegotiatedInbound(FullyNegotiatedInbound {
+                protocol: EitherOutput::First(protocol),
+                info: Either::Left(info),
+            }) => Ok(Either::Left(ConnectionEvent::FullyNegotiatedInbound(
+                FullyNegotiatedInbound { protocol, info },
+            ))),


Suggested change

ConnectionEvent::FullyNegotiatedInbound(FullyNegotiatedInbound {

protocol: EitherOutput::First(protocol),

info: Either::Left(info),

}) => Ok(Either::Left(ConnectionEvent::FullyNegotiatedInbound(

FullyNegotiatedInbound { protocol, info },

))),

ConnectionEvent::FullyNegotiatedInbound(inner) => Ok(inner.transpose()),

Does this work? Or do we have to call .map on it again or something?

It would be cool if we can express this transpose function somehow through the smaller ones.

It doesn't because Select's implementation uses SelectUpgrade whereas Either uses EitherUpgrade, do you have any idea how can we combine them in transpose? :P

I don't think we can combine them for select and either because those are fundamentally different.

Also, one of them is using EitherOutput which we need because Either doesn't implement AsyncRead + AsyncWrite.

See #2650.

…ject-calls

mergify · 2023-01-02T20:22:48Z

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

…ject-calls

jxs · 2023-01-05T18:49:28Z

@thomaseizinger submitted #3307 which started as fork from this one and also addresses some of the conversations here. Can we merge this one and continue there? I can also open an issue to address potential unresolved conversations.

thomaseizinger

A few more comments, overall looks good to me!

thomaseizinger · 2023-01-11T04:45:29Z

swarm/CHANGELOG.md

 - Add `estblished_in` to `SwarmEvent::ConnectionEstablished`. See [PR 3134].

+- Remove deprecated `inject_*` methods from `NetworkBehaviour` and `ConnectionHandler`.
+  see [PR 3260].


Suggested change

see [PR 3260].

see [PR 3264].

Wrong issue reference :)

thomaseizinger · 2023-01-11T04:50:23Z

swarm/src/handler/either.rs

+            ConnectionEvent::FullyNegotiatedInbound(FullyNegotiatedInbound {
+                protocol: EitherOutput::First(protocol),
+                info: Either::Left(info),
+            }) => Ok(Either::Left(ConnectionEvent::FullyNegotiatedInbound(
+                FullyNegotiatedInbound { protocol, info },
+            ))),


I don't think we can combine them for select and either because those are fundamentally different.

thomaseizinger · 2023-01-11T04:56:33Z

swarm/src/handler/either.rs

+            ConnectionEvent::FullyNegotiatedInbound(FullyNegotiatedInbound {
+                protocol: EitherOutput::First(protocol),
+                info: Either::Left(info),
+            }) => Ok(Either::Left(ConnectionEvent::FullyNegotiatedInbound(
+                FullyNegotiatedInbound { protocol, info },
+            ))),


Also, one of them is using EitherOutput which we need because Either doesn't implement AsyncRead + AsyncWrite.

See #2650.

thomaseizinger · 2023-01-11T05:05:17Z

swarm/src/handler/select.rs

+                match fully_negotiated_outbound.transpose() {
+                    Either::Left(f) => self
+                        .proto1
+                        .on_connection_event(ConnectionEvent::FullyNegotiatedOutbound(f)),
+                    Either::Right(f) => self
+                        .proto2
+                        .on_connection_event(ConnectionEvent::FullyNegotiatedOutbound(f)),
+                }
            }
            ConnectionEvent::FullyNegotiatedInbound(fully_negotiated_inbound) => {
-                self.on_fully_negotiated_inbound(fully_negotiated_inbound)
+                match fully_negotiated_inbound.transpose() {
+                    Either::Left(f) => self
+                        .proto1
+                        .on_connection_event(ConnectionEvent::FullyNegotiatedInbound(f)),
+                    Either::Right(f) => self
+                        .proto2
+                        .on_connection_event(ConnectionEvent::FullyNegotiatedInbound(f)),
+                }
            }
            ConnectionEvent::AddressChange(address) => {
-                #[allow(deprecated)]
-                self.proto1.inject_address_change(address.new_address);
-                #[allow(deprecated)]
-                self.proto2.inject_address_change(address.new_address)
+                self.proto1
+                    .on_connection_event(ConnectionEvent::AddressChange(AddressChange {
+                        new_address: address.new_address,
+                    }));
+
+                self.proto2
+                    .on_connection_event(ConnectionEvent::AddressChange(AddressChange {
+                        new_address: address.new_address,
+                    }));
            }
            ConnectionEvent::DialUpgradeError(dial_upgrade_error) => {
-                self.on_dial_upgrade_error(dial_upgrade_error)
+                match dial_upgrade_error.transpose() {
+                    Either::Left(err) => self
+                        .proto1
+                        .on_connection_event(ConnectionEvent::DialUpgradeError(err)),
+                    Either::Right(err) => self
+                        .proto2
+                        .on_connection_event(ConnectionEvent::DialUpgradeError(err)),
+                }
            }


I wonder if it would be better to do it like this for the either::Handler as well? i.e. only transpose the inner type instead of the entire event and matching on the tuple.

Yeah you are right Thomas, you had already mentioned before sorry it slipped me. It makes sense cause then we don't need Err for AddressChange. Updated ptal!

do it per variant instead of on the ConnectionEvent itself, so that we no longer need the Err on the case of AddressChange.

thomaseizinger

LGTM

Nice to see it being a negative diff!

Sorry I missed this on #3264

jxs added 5 commits December 17, 2022 21:38

rendezvous: rename SubstreamHandler::inject_event

c0bf884

to on_event

rendezvous: replace inject_* methods with on_* on SubstreamHandler.

29f4882

dcutr: remove dangling inject_ methods for Handler

61d3535

swarm: remove inject_ methods from ConnectionHandler.

500283a

swarm: remove inject_ methods from NetworkBehaviour,

d29a8c9

swarm-derive: remove inject methods from derive macro.

jxs changed the title ~~Remove inject calls from swarm~~ refactor: remove inject calls from swarm Dec 20, 2022

jxs changed the title ~~refactor: remove inject calls from swarm~~ refactor(swarm): remove deprecated inject calls Dec 20, 2022

thomaseizinger reviewed Dec 20, 2022

View reviewed changes

thomaseizinger mentioned this pull request Dec 21, 2022

feat(swarm)!: allow NetworkBehaviours to manage connections #3254

Merged

4 tasks

mxinden reviewed Dec 21, 2022

View reviewed changes

jxs added 2 commits December 21, 2022 16:13

update swarm-derive CHANGELOG and Cargo.toml.

354257e

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

ab14438

…ject-calls

jxs commented Dec 21, 2022

View reviewed changes

mxinden reviewed Dec 22, 2022

View reviewed changes

mxinden approved these changes Dec 22, 2022

View reviewed changes

jxs added 2 commits December 22, 2022 22:38

review: implement transpose functions

0831cba

for ConnectionEvent and DialUpgradeError, to help when matching on them for Either and Select.

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

264b27f

…ject-calls

thomaseizinger reviewed Dec 22, 2022

View reviewed changes

jxs added 2 commits December 23, 2022 15:28

review: implement transpose on swarm::handler::select,

b9afb31

for the remaining ConnectionEvents that is useful to implement on.

review: change ConnectionEvent::transpose return type

2325c6b

to Result and document it.

thomaseizinger reviewed Dec 24, 2022

View reviewed changes

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

49162b4

…ject-calls

jxs mentioned this pull request Jan 2, 2023

swarm/handler: Split inject_listen_upgrade_error into two #2894

Closed

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

a0af32b

…ject-calls

thomaseizinger approved these changes Jan 11, 2023

View reviewed changes

review: fix PR typo in changelog

e2e8d93

review: update Select transpose of ConnectionEvent

a6c052d

do it per variant instead of on the ConnectionEvent itself, so that we no longer need the Err on the case of AddressChange.

jxs force-pushed the remove-inject-calls branch from 017641e to a6c052d Compare January 11, 2023 15:50

thomaseizinger approved these changes Jan 11, 2023

View reviewed changes

mxinden added the send-it label Jan 12, 2023

Merge branch 'master' into remove-inject-calls

6232ff6

mergify bot merged commit 4c65c7d into libp2p:master Jan 12, 2023

jxs mentioned this pull request Jan 20, 2023

fix(swarm)!: make on_connection_handler_event impl mandatory. #3364

Merged

4 tasks

mergify bot pushed a commit that referenced this pull request Jan 24, 2023

fix(swarm)!: make on_connection_handler_event impl mandatory. (#3364)

d7ee0e4

Sorry I missed this on #3264

		fn on_swarm_event(&mut self, _event: FromSwarm<Self::ConnectionHandler>) {}
		fn on_swarm_event(&mut self, event: FromSwarm<Self::ConnectionHandler>);

		- Replace `NetworkBehaviour` Derive macro deprecated `inject_*` method implementations
		with the new `on_swarm_event` and `on_connection_handler_event`.

refactor(swarm): remove deprecated inject calls #3264

refactor(swarm): remove deprecated inject calls #3264

Uh oh!

Conversation

jxs commented Dec 20, 2022

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

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!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 23, 2022

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!

mergify bot commented Jan 2, 2023

Uh oh!

jxs commented Jan 5, 2023

Uh oh!

thomaseizinger left a comment