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

feat(swarm): Add AndBehaviour #3366

Closed
wants to merge 13 commits into from
Closed

feat(swarm): Add AndBehaviour #3366

wants to merge 13 commits into from

Conversation

jxs
Copy link
Member

@jxs jxs commented Jan 21, 2023

Description

Addresses #2719.

Notes

I started this by basically following the code generated by a NetworkBehaviour derive implementation for a struct similar to And (but without generic fields) but then it seemed a lot of similar code to the generated macro to maintain.

I then followed to the approach submitted on this PR, have the And struct itself the NetworkBehaviour derived. While going for this approach I found a bug with the NetworkBehaviour derive implementation for generics, when out_event was not provided the enum generated missed the NetworkBehaviour impl constraint for the generic variants whilst using the generics for <Generic>::OutEvent. This is what a4570af addresses.

The manual Debug implementation was required because the Debug derive is not smart enough, when we annotate it on a struct with generics it requires it's generics to impl Debug but on our case what we want is that only the OutEvent associated type of the generic variants implement Debug. see similar here or here

The derive macro shouldn't be breaking, from what I tested the printed debug output is the same for non generic types.
I updated the ping example to use And, if agreed will update the other examples.
If this seems to hacky I can rollback and submit instead the first approach.

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

@jxs jxs marked this pull request as draft January 21, 2023 19:04
@thomaseizinger
Copy link
Contributor

Hmm, I think the fix is worth landing (maybe even in a separate PR?).

If And is this trivial to implement, I don't think we need to provide it? Not sure. It might be a nice utility to have. Perhaps we should be implementing NetworkBehaviour for tuples?

@jxs jxs force-pushed the and-behaviour branch 2 times, most recently from 32d2f44 to e8d6e00 Compare January 22, 2023 00:22
@jxs jxs changed the title feat(swarm)!: Add AndBehaviour feat(swarm): Add AndBehaviour Jan 22, 2023
@jxs jxs force-pushed the and-behaviour branch 2 times, most recently from fff9e31 to 910f7c7 Compare January 23, 2023 10:46
for generic types when out_event was not provided. Previously The enum generated
didn't have the NetworkBehaviour impl constraints whilst using the generics for <Generic>::OutEvent.
instead of a custom structure that derives `NetworkBehaviour`.
@jxs jxs marked this pull request as ready for review January 23, 2023 11:58
@jxs
Copy link
Member Author

jxs commented Jan 23, 2023

Good question!
IIUC you are suggesting tupples so we can have more than two elements right?
AFACT to implement NetworkBehaviour for tuples it seems we would need to do it manually, (see here or here how std does it for its traits) instead of deriving.

So it seems a matter of choosing where to have more complexity, either in the code (manually implementation) or in the user API, <And<A, And<B, C>> which would mean and.second_as_ref().first_as_ref() to access it's behaviours, and AndEvent(AndEvent(Behaviour) vs tuple.2 and TupleEvent(Behaviour).
No strong opinion on my end Thomas.

@mergify
Copy link
Contributor

mergify bot commented Jan 23, 2023

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

@thomaseizinger
Copy link
Contributor

My vote would go against providing an And type. The implementation is so trivial, users can just create it themselves when they need it, esp. if they need to compose more than two behaviours.

@mergify
Copy link
Contributor

mergify bot commented Jan 24, 2023

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

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still in favor of adding an And NetworkBehaviour, even though it is very simple. Not a strong opinion.

Can you please extract the swarm-derive fix into a separate pull request @jxs?

@@ -1,9 +1,14 @@
# 0.32.0 [unreleased]

- Fix `NetworkBehaviour` Derive macro for generic types when `out_event` was not provided. Previously The enum generated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Fix `NetworkBehaviour` Derive macro for generic types when `out_event` was not provided. Previously The enum generated
- Fix `NetworkBehaviour` Derive macro for generic types when `out_event` was not provided. Previously the enum generated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Max, addressed in #3393

@@ -1,5 +1,10 @@
# 0.42.0 [unreleased]

- Add `And` struct that combines two `NetworkBehaviour` implementations. Helfpful for when one wants
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Add `And` struct that combines two `NetworkBehaviour` implementations. Helfpful for when one wants
- Add `And` struct that combines two `NetworkBehaviour` implementations. Helpful for when one wants

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! updated!

Comment on lines 44 to 51
/// Implementation of [`NetworkBehaviour`] that combines two underlying implementations.
#[cfg(feature = "macros")]
#[derive(Debug, Default, libp2p_swarm_derive::NetworkBehaviour)]
#[behaviour(prelude = "crate::derive_prelude")]
pub struct And<A, B> {
first: A,
second: B,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NetworkBehaviour combinators like Toggle and Either are in their own file. I would suggest doing the same for And.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Addressed!

@thomaseizinger
Copy link
Contributor

I am still in favor of adding an And NetworkBehaviour, even though it is very simple. Not a strong opinion.

Can you please extract the swarm-derive fix into a separate pull request @jxs?

If we add it, I think the fields should just be public to make access easier.

@@ -40,6 +41,47 @@ pub(crate) type THandlerInEvent<THandler> =
pub(crate) type THandlerOutEvent<THandler> =
<<THandler as IntoConnectionHandler>::Handler as ConnectionHandler>::OutEvent;

/// Implementation of [`NetworkBehaviour`] that combines two underlying implementations.
#[cfg(feature = "macros")]
#[derive(Debug, Default, libp2p_swarm_derive::NetworkBehaviour)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now this will have the macro-generated OutEvent looking kinda like this:

enum AndEvent<A, B> {
    First(A)
    Second(B)
}

What do you think about specifying Either<A, B> as out-event instead? Would also be more consistent with all of our other "combinators".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Note @jxs that you can still achieve the above suggestion with the derive macro by using the out_event derive parameter, see https://docs.rs/libp2p-swarm/latest/libp2p_swarm/behaviour/trait.NetworkBehaviour.html#custom-networkbehaviour-with-the-derive-macro.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I tried that while experimenting, but when you specify a custom out_event one has to impl From for each subvariant. In case of using Either the generated code is the following:

    impl<A, B> crate::derive_prelude::NetworkBehaviour for And<A, B>
    where
        A: crate::derive_prelude::NetworkBehaviour,
        B: crate::derive_prelude::NetworkBehaviour,
        Either<A, B>: From<<A as crate::derive_prelude::NetworkBehaviour>::OutEvent>,
        Either<A, B>: From<<B as crate::derive_prelude::NetworkBehaviour>::OutEvent>,

which kinda doesn't make sense as it would require for us to know A and B, and means that specifying out_event for a derived NetworkBehaviour with generics is not supported now. Thanks for referring that Elena. I am kind of out of ideas to allow derive to support custom out_event for generic structures. Do you ave any suggestion? Else I can add this to the doc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes you're right. Using Either as out-event would only be possible if we manually derive NetworkBehaviour. I don't feel strongly about it, thus I'm fine with sticking with the current solution.

I am kind of out of ideas to allow derive to support custom out_event for generic structures. Do you ave any suggestion?

I don't think its possible. We will always run into the issue of conflicting implementations if we try to auto-implement From, since A and B theoretically may have the same type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a point for not providing an And behaviour. If a user wants to group a set of NetworkBehaviours together, they can do so themselves and provide a more meaningful name than And. Like, if they combine certain behaviours together than are only available to under a "legacy" feature-toggle, they can call this behaviour Legacy and the generated name will make more sense.

Plus, they can add as many behaviours as they want, not just two, they can decide on the visibility, they can decide on the names etc.

I really don't see the value in providing an And behaviour.

@jxs
Copy link
Member Author

jxs commented Jan 27, 2023

Opened #3393

I am still in favor of adding an And NetworkBehaviour, even though it is very simple. Not a strong opinion.
Can you please extract the swarm-derive fix into a separate pull request @jxs?

If we add it, I think the fields should just be public to make access easier.

``

I am still in favor of adding an And NetworkBehaviour, even though it is very simple. Not a strong opinion.
Can you please extract the swarm-derive fix into a separate pull request @jxs?

If we add it, I think the fields should just be public to make access easier.

Yeah, it's the dilemma regarding being allowed to make changes without breaking API easily vs easier to access API, funny enough I thought you'd go for the first 😁 Updated!

@jxs
Copy link
Member Author

jxs commented Jan 27, 2023

meanwhile opened #3393 but forgot to open it from a libp2p/rust-libp2p repo so we could then make this one target it sorry. After that one gets merged this one will be updated.

use ReadyUpgrade for Handler::InboundProtocol
use ReadyUpgrade for Handler::OutboundProtocol.
@mergify
Copy link
Contributor

mergify bot commented Feb 1, 2023

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

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against adding this. See discussion: #3366 (comment).

@mergify
Copy link
Contributor

mergify bot commented Feb 14, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants