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

swarm/src/behaviour: Deprecate NetworkBehaviourEventProcess #2784

Merged
merged 12 commits into from
Aug 16, 2022
67 changes: 67 additions & 0 deletions swarm/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,72 @@
# 0.38.0 [unreleased]

- Deprecate `NetworkBehaviourEventProcess`. When deriving `NetworkBehaviour` on a custom `struct` users
should bring their own `OutEvent` via `#[behaviour(out_event = "MyBehaviourEvent")]`.

See [`NetworkBehaviour`
documentation](https://docs.rs/libp2p/latest/libp2p/swarm/trait.NetworkBehaviour.html) for
details.

mxinden marked this conversation as resolved.
Show resolved Hide resolved
Previously

``` rust
#[derive(NetworkBehaviour)]
#[behaviour(event_process = true)]
struct MyBehaviour {
floodsub: Gossipsub,
mxinden marked this conversation as resolved.
Show resolved Hide resolved
mdns: Mdns,
}

impl NetworkBehaviourEventProcess<Gossipsub> for MyBehaviour {
fn inject_event(&mut self, message: GossipsubEvent) {
todo!("Handle event")
}
}

impl NetworkBehaviourEventProcess<MdnsEvent> for MyBehaviour {
fn inject_event(&mut self, message: MdnsEvent) {
todo!("Handle event")
}
}
```

Now

``` rust
#[derive(NetworkBehaviour)]
#[behaviour(out_event = "MyBehaviourEvent")]
struct MyBehaviour {
floodsub: Gossipsub,
mxinden marked this conversation as resolved.
Show resolved Hide resolved
mdns: Mdns,
}

enum MyBehaviourEvent {
Floodsub(FloodsubEvent),
mxinden marked this conversation as resolved.
Show resolved Hide resolved
Mdns(MdnsEvent),
}

impl From<GossipsubEvent> for MyBehaviourEvent {
fn from(event: GossipsubEvent) -> Self {
MyBehaviourEvent::Gossipsub(event)
}
}

impl From<MdnsEvent> for MyBehaviourEvent {
fn from(event: MdnsEvent) -> Self {
MyBehaviourEvent::Mdns(event)
}
}

match swarm.next().await.unwrap() {
SwarmEvent::Behaviour(MyBehaviourEvent::Gossipsub(event)) => {
todo!("Handle event")
}
SwarmEvent::Behaviour(MyBehaviourEvent::Mdns(event)) => {
todo!("Handle event")
}
}
```

- Update dial address concurrency factor to `8`, thus dialing up to 8 addresses concurrently for a single connection attempt. See `Swarm::dial_concurrency_factor` and [PR 2741].

- Update to `libp2p-core` `v0.35.0`.
Expand Down
30 changes: 12 additions & 18 deletions swarm/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,11 @@ pub(crate) type THandlerOutEvent<THandler> =
/// it will delegate to each `struct` member and return a concatenated array of all addresses
/// returned by the struct members.
///
/// When creating a custom [`NetworkBehaviour`], you must choose one of two methods to respond to
/// incoming events:
/// * One option is setting a custom `out_event` with `#[behaviour(out_event = "AnotherType")]`.
/// In this case, events generated by the custom [`NetworkBehaviour`] struct members will be
/// converted to your custom `out_event` for you to handle after polling the swarm.
/// * Alternatively, users that need access to the root [`NetworkBehaviour`] implementation while
/// processing emitted events, can specify `#[behaviour(event_process = true)]` (default is false).
/// Events generated by the behaviour's struct members are delegated to [`NetworkBehaviourEventProcess`]
/// trait implementations. Those must be provided by the user on the type that [`NetworkBehaviour`]
/// is derived on.
/// Events ([`NetworkBehaviour::OutEvent`]) returned by each `struct` member are wrapped in a new
/// `enum` event, with an `enum` variant for each `struct` member. Users can define this event
/// `enum` themselves and provide the name to the derive macro via `#[behaviour(out_event =
/// "MyCustomOutEvent")]`. If the user does not specify an `out_event`, the derive macro generates
/// the event definition itself, naming it `<STRUCT_NAME>Event`.
elenaf9 marked this conversation as resolved.
Show resolved Hide resolved
///
/// When setting a custom `out_event`, the aforementioned conversion of each of the event types
/// generated by the struct members to the custom `out_event` is handled by [`From`]
Expand Down Expand Up @@ -123,14 +118,6 @@ pub(crate) type THandlerOutEvent<THandler> =
/// }
/// ```
///
/// When using `event_process = true` the [`NetworkBehaviourEventProcess`] trait implementations
/// are granted exclusive access to the [`NetworkBehaviour`], therefore
/// [blocking code](https://ryhl.io/blog/async-what-is-blocking/) in these implementations will
/// block the entire [`Swarm`](crate::Swarm) from processing new events, since the swarm cannot progress
/// without also having exclusive access to the [`NetworkBehaviour`]. A better alternative is to execute
/// blocking or asynchronous logic on a separate task, perhaps with the help of a bounded channel to
/// maintain backpressure. The sender for the channel could be included in the NetworkBehaviours constructor.
///
/// Optionally one can provide a custom `poll` function through the `#[behaviour(poll_method =
/// "poll")]` attribute. This function must have the same signature as the [`NetworkBehaviour#poll`]
/// function and will be called last within the generated [`NetworkBehaviour`] implementation.
Expand Down Expand Up @@ -340,6 +327,13 @@ pub trait PollParameters {
///
/// You can opt out of this behaviour through `#[behaviour(event_process = false)]`. See the
/// documentation of [`NetworkBehaviour`] for details.
#[deprecated(
since = "0.38.0",
note = "Use `#[behaviour(out_event = \"MyBehaviourEvent\")]` instead. See \
https://github.com/libp2p/rust-libp2p/blob/master/swarm/CHANGELOG.md#0380 \
mxinden marked this conversation as resolved.
Show resolved Hide resolved
for instructions on how to migrate. Will be removed with \
https://github.com/libp2p/rust-libp2p/pull/2751."
)]
pub trait NetworkBehaviourEventProcess<TEvent> {
/// Called when one of the fields of the type you're deriving `NetworkBehaviour` on generates
/// an event.
Expand Down
8 changes: 4 additions & 4 deletions swarm/src/behaviour/either.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,9 @@
// DEALINGS IN THE SOFTWARE.

use crate::handler::{either::IntoEitherHandler, ConnectionHandler, IntoConnectionHandler};
use crate::{
DialError, NetworkBehaviour, NetworkBehaviourAction, NetworkBehaviourEventProcess,
PollParameters,
};
#[allow(deprecated)]
pub use crate::NetworkBehaviourEventProcess;
use crate::{DialError, NetworkBehaviour, NetworkBehaviourAction, PollParameters};
use either::Either;
use libp2p_core::{
connection::ConnectionId, transport::ListenerId, ConnectedPoint, Multiaddr, PeerId,
Expand Down Expand Up @@ -237,6 +236,7 @@ where
}
}

#[allow(deprecated)]
impl<TEvent, TBehaviourLeft, TBehaviourRight> NetworkBehaviourEventProcess<TEvent>
for Either<TBehaviourLeft, TBehaviourRight>
where
Comment on lines +239 to 242
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the usage case for NetworkBehaviourEventProcess on Either (or Toggle below)?
As far as I understood it, the purpose of NetworkBehaviourEventProcess is only to be used in combination with the NetworkBehaviour 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.

I guess when you use Either or Toggle when building a nested NetworkBehaviour. See for example:

#[test]
fn with_toggle() {
use libp2p::swarm::behaviour::toggle::Toggle;
#[allow(dead_code)]
#[derive(NetworkBehaviour)]
struct Foo {
identify: libp2p::identify::Identify,
ping: Toggle<libp2p::ping::Ping>,
}
#[allow(dead_code)]
fn foo() {
require_net_behaviour::<Foo>();
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Here with NetworkBehaviourEventProcess:

#[test]
fn with_toggle() {
use libp2p::swarm::behaviour::toggle::Toggle;
#[allow(dead_code)]
#[derive(NetworkBehaviour)]
#[behaviour(event_process = true)]
struct Foo {
identify: libp2p::identify::Identify,
ping: Toggle<libp2p::ping::Ping>,
}
impl libp2p::swarm::NetworkBehaviourEventProcess<libp2p::identify::IdentifyEvent> for Foo {
fn inject_event(&mut self, _: libp2p::identify::IdentifyEvent) {}
}
impl libp2p::swarm::NetworkBehaviourEventProcess<libp2p::ping::PingEvent> for Foo {
fn inject_event(&mut self, _: libp2p::ping::PingEvent) {}
}
#[allow(dead_code)]
fn foo() {
require_net_behaviour::<Foo>();
}
}

Copy link
Contributor

@elenaf9 elenaf9 Aug 10, 2022

Choose a reason for hiding this comment

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

But you wouldn't call the NetworkBehaviourEventProcess implementation of an inner behaviour, no? Instead you implement NetworkBehaviourEventProcess on the parent (in the above case in Foo) and handle the event there. Why would anyone want to inject the event back into an inner behaviour?

None of our behaviours currently implement NetworkBehaviourProcess, so the below trait bound for it would not apply anyway:

where
TBehaviour: NetworkBehaviourEventProcess<TEvent>,
{

So libp2p::ping::Ping does not implement NetworkBehaviourEventProcess, therefore Toggle<libp2p::ping::Ping> will also not implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am missing something, there was never really a use-case for NetworkBehaviourEventProcess on those two behaviours in the first place. However, that's not really a concern of this PR.
So I guess I am fine with for now with keeping it the way it is, since this trait will be remove anyway in the next release.

Expand Down
8 changes: 4 additions & 4 deletions swarm/src/behaviour/toggle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@ use crate::handler::{
KeepAlive, SubstreamProtocol,
};
use crate::upgrade::{InboundUpgradeSend, OutboundUpgradeSend, SendWrapper};
use crate::{
DialError, NetworkBehaviour, NetworkBehaviourAction, NetworkBehaviourEventProcess,
PollParameters,
};
#[allow(deprecated)]
pub use crate::NetworkBehaviourEventProcess;
use crate::{DialError, NetworkBehaviour, NetworkBehaviourAction, PollParameters};
use either::Either;
use libp2p_core::{
connection::ConnectionId,
Expand Down Expand Up @@ -233,6 +232,7 @@ where
}
}

#[allow(deprecated)]
impl<TEvent, TBehaviour> NetworkBehaviourEventProcess<TEvent> for Toggle<TBehaviour>
where
TBehaviour: NetworkBehaviourEventProcess<TEvent>,
Expand Down
5 changes: 3 additions & 2 deletions swarm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ pub mod behaviour;
pub mod dial_opts;
pub mod handler;

#[allow(deprecated)]
pub use behaviour::NetworkBehaviourEventProcess;
pub use behaviour::{
CloseConnection, NetworkBehaviour, NetworkBehaviourAction, NetworkBehaviourEventProcess,
NotifyHandler, PollParameters,
CloseConnection, NetworkBehaviour, NetworkBehaviourAction, NotifyHandler, PollParameters,
};
pub use connection::{
ConnectionCounters, ConnectionError, ConnectionLimit, ConnectionLimits, PendingConnectionError,
Expand Down