swarm/behaviour: Replace `inject_*` with `on_event` by jxs · Pull Request #3011 · libp2p/rust-libp2p

jxs · 2022-10-11T16:34:37Z

Description

Superseeds #2867.

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

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

…ehaviour-on-event

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

jxs

Question:

In the NetworkBehaviour doc, some inject_* (now on_*) refer Indicates to the behaviour others informs the behaviour should we have them all on the same wording, for consistency?
updated the autonat behaviour, if that looks ok to you will follow to the other protocols

…-on-event

mxinden · 2022-10-11T16:56:55Z

In the NetworkBehaviour doc, some inject_* (now on_*) refer Indicates to the behaviour others informs the behaviour should we have them all on the same wording, for consistency?

Consistent wording sounds good to me. Thanks for catching that. I suggest only changing the wording on the new APIs.

jxs · 2022-10-11T21:29:57Z

In the NetworkBehaviour doc, some inject_* (now on_*) refer Indicates to the behaviour others informs the behaviour should we have them all on the same wording, for consistency?

Consistent wording sounds good to me. Thanks for catching that. I suggest only changing the wording on the new APIs.

sorry, forgot to ask which one should prevail then

…-on-event

thomaseizinger · 2022-10-12T03:46:00Z

In the NetworkBehaviour doc, some inject_* (now on_*) refer Indicates to the behaviour others informs the behaviour should we have them all on the same wording, for consistency?

Consistent wording sounds good to me. Thanks for catching that. I suggest only changing the wording on the new APIs.

sorry, forgot to ask which one should prevail then

"inform" sounds better to me but I don't have a strong opinion here.

on on_connection_handler_event

to use new FromSwarm variant structs

on NetworkBehaviour implementation

…-on-event

mxinden · 2022-11-12T12:34:11Z

I suggest we merge this together with #3085 to make sure both land in the same release. Objections?

thomaseizinger · 2022-11-12T23:44:01Z

I suggest we merge this together with #3085 to make sure both land in the same release. Objections?

That would be my preference too!

mergify · 2022-11-13T00:01:01Z

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

mxinden · 2022-11-14T09:35:44Z

-                    remaining_established,
-                ),
-            _ => unreachable!(),
+            Either::Left(b) => b.on_swarm_event(event.map_handler(


@thomaseizinger comment on #3085 (comment) sparked a thought.

What if b implements the inject_* style methods and not the on_* style methods? In such case, this call would be lost, right?

Unless I am missing something combinators like Either need to call the inject_* style methods until we remove them.

No no, I noticed it when replacing Inject_* on combinators for #3085 but with the deprecation/removal discussion forgot to address it here, thanks for reminding @mxinden! Updated to call inject methods on those structs. Also merged from master and fixed conflicts ptal.

…-on-event

mergify · 2022-11-14T22:37:05Z

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

mxinden

I am afraid there is still one larger work item on swarm-derive. Otherwise this pull request looks good to me.

mxinden · 2022-11-15T10:45:41Z

+            .iter()
+            .enumerate()
+            .map(|(field_n, field)| match field.ident {
+                Some(ref i) => quote! { self.#i.on_swarm_event(


swarm-derive will need to delegate to the inject_* methods of the corresponding NetworkBehaviour implementations, right? Otherwise our non-breaking deprecation doesn't work.

yeah right, thanks for noticing Max, I think I addressed all the cases but ptal.

…-on-event-2

mxinden · 2022-11-15T14:17:49Z

Thanks for the follow-ups. Once the last comments in #3085 (review) are addressed, we can merge both.

mergify · 2022-11-15T14:27:50Z

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

…-on-event-2

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

#3085 is all approved. Adding send-it here.

`libp2p-swarm-derive` released a breaking change (libp2p#3011) as a patch release `v0.30.2`. This patch: 1. Prepares a new minor release of `libp2p-swarm-derive`. 2. Prepares a patch release for `libp2p-swarm` to use the minor release of `libp2p-swarm-derive`. As a follow up we can yank the `libp2p-swarm-derive` `v0.30.2` release. In addition we might want to release `libp2p-swarm-derive` `v0.30.3` containing all patches but libp2p#3011.

`libp2p-swarm-derive` released a breaking change (#3011) as a patch release `v0.30.2`. This patch: 1. Prepares a new minor release of `libp2p-swarm-derive`. 2. Prepares a patch release for `libp2p-swarm` to use the minor release of `libp2p-swarm-derive`. As a follow up we can yank the `libp2p-swarm-derive` `v0.30.2` release. In addition we might want to release `libp2p-swarm-derive` `v0.30.3` containing all patches but #3011.

nazar-pc · 2022-12-01T07:59:31Z

+
+/// Enumeration with the list of the possible events
+/// to pass to [`on_swarm_event`](NetworkBehaviour::on_swarm_event).
+pub enum FromSwarm<'a, Handler: IntoConnectionHandler> {


Why isn't there Clone derive? I need to forward this to underlying protocols and it seems like I need to match on all variants just to be able to clone and wrap it back into FromSwarm or am I missing something?

There are multiple variants that contain a ConnectionHandler which can't be cloned.

See #3046.

Sometimes it feels like we're going through a lot of painful contortions just to avoid saying Arc.

Sometimes it feels like we're going through a lot of painful contortions just to avoid saying Arc.

In this case, we would also have to say try_unwrap: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.try_unwrap

Possible but doesn't feel very clean. I don't particularly like both solutions but I think we are getting one step closer with the new event into making composition easier.

#3099 will help with this too because we are removing a few more mentions of the handler there.

mxinden and others added 11 commits September 4, 2022 12:53

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`.

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

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.

review: address suggestions

1a33678

autonat/behaviour: Replace inject_* with on_event

09ca723

jxs commented Oct 11, 2022

View reviewed changes

Comment thread swarm/src/behaviour.rs Outdated

Comment thread swarm/src/behaviour.rs

Comment thread swarm/src/handler/either.rs

Comment thread swarm/tests/public_api.rs Outdated

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

af7cf9b

…-on-event

mxinden reviewed Oct 11, 2022

View reviewed changes

Comment thread protocols/autonat/src/behaviour.rs Outdated

jxs added 2 commits October 11, 2022 22:35

swarm: remove no longer required DummyBehaviour

4f5cc26

autonat/behaviour: remove no longer needed inject methods

510fd5e

jxs force-pushed the behaviour-on-event branch from c3e648e to 510fd5e Compare October 11, 2022 21:37

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

9bb6714

…-on-event

thomaseizinger reviewed Oct 12, 2022

View reviewed changes

Comment thread protocols/autonat/src/behaviour.rs Outdated

thomaseizinger reviewed Oct 12, 2022

View reviewed changes

Comment thread swarm/src/behaviour.rs Outdated

swarm/behaviour: use full definition instead of type alias,

9b60343

on on_connection_handler_event

mxinden mentioned this pull request Oct 12, 2022

swarm/behaviour: Replace inject_* with on_event #2867

Closed

4 tasks

jxs added 5 commits October 13, 2022 15:48

Merge branch 'master' into behaviour-on-event

b3743da

swarm/behaviour: update FromSwarm enum variants to structs

feccba0

autonat/behaviour: update on_swarm_event,

6dc84f8

to use new FromSwarm variant structs

request-response/lib: replace inject_* methods with on_*,

e28fac5

on NetworkBehaviour implementation

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

80b2345

…-on-event

jxs force-pushed the behaviour-on-event branch from 6390f31 to b9baeb3 Compare November 7, 2022 23:34

Merge branch 'master' into behaviour-on-event

bb15c1f

mxinden reviewed Nov 14, 2022

View reviewed changes

jxs added 3 commits November 14, 2022 10:27

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

ed3f5cf

…-on-event

review: revert to call inject on inner types.

73e0dcf

Merge branch 'behaviour-on-event' into behaviour-on-event-2

10a5a62

mxinden reviewed Nov 15, 2022

View reviewed changes

jxs added 3 commits November 15, 2022 11:25

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

eb307bd

…-on-event-2

review: revert swarm-derive to call inject on inner types.

f0fd82a

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

d11950b

…-on-event-2

jxs force-pushed the behaviour-on-event branch from 1a662cb to f0fd82a Compare November 15, 2022 12:39

review: revert missing var names on swarm-derive.

dcf8235

mxinden approved these changes Nov 15, 2022

View reviewed changes

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

cc2b56e

…-on-event-2

mxinden added the send-it label Nov 17, 2022

Merge branch 'master' into behaviour-on-event

8604715

mergify Bot merged commit 3df3c88 into libp2p:master Nov 17, 2022

thomaseizinger mentioned this pull request Nov 29, 2022

Unable to use libp2p 0.49.0 plus libp2p-swarm-derive while publishing a library #3176

Closed

mxinden mentioned this pull request Nov 29, 2022

fix(derive): Release breaking change as v0.31.0 #3178

Merged

4 tasks

nazar-pc reviewed Dec 1, 2022

View reviewed changes

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024

swarm/behaviour: Replace inject_* with on_event (libp2p#3011)

4b5f4e2

Conversation

jxs commented Oct 11, 2022

Description

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

jxs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxinden commented Oct 11, 2022

Uh oh!

Uh oh!

jxs commented Oct 11, 2022

Uh oh!

thomaseizinger commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

mxinden commented Nov 12, 2022

Uh oh!

thomaseizinger commented Nov 12, 2022

Uh oh!

mergify Bot commented Nov 13, 2022

Uh oh!

mxinden Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify Bot commented Nov 14, 2022

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

jxs Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

mxinden commented Nov 15, 2022

Uh oh!

mergify Bot commented Nov 15, 2022

Uh oh!

mxinden commented Nov 17, 2022

Uh oh!

nazar-pc Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

rkuhn Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Dec 1, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jxs left a comment •

edited

Loading

jxs Nov 14, 2022 •

edited

Loading