swarm/src/behaviour: Deprecate NetworkBehaviourEventProcess

[mxinden]

Description

In preparation for #2751.

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

[thomaseizinger]

Concept ACK, just one suggestion.

[elenaf9]

If I understand it correctly, the plan is to publish a release that includes this changes here (deprecating NetworkBehaviourEventProcess), but do a separate release for the main changes of #2751 (changing the proc macro, removing NetworkBehaviourEventProcess completely).

I am not sure if it make sense to deprecate NetworkBehaviourEventProcess without publishing the changes in the proc macro. User that want to upgrade can not use the automatically generated OutEvent yet. Imo ideally the first release should already include all changes of #2751, but not yet remove support for NetworkBehaviourEventProcess / event_process=false event_process=true.
The problem that I see here is what the default should be (= What should be generated if a struct is only annotated with #[derive(NetworkBehavour)] and nothing else?).
In the old version the default was to expect NetworkBehaviourEventProcess, in the new version the default will be to generate the OutEvent enum.
What do you think about the following:

• Include all changes of #2751, but only deprecate NetworkBehaviourEventProcess instead of removing it and keep support for event_process=false event_process=true (which means that for this interim period the macro would have to support both, event_process and the new generated OutEvent)
• Change default for event_process to true instead of false. User that would like to keep using it would have to explicitly add #[event_process=false] rather than it being the default
• Per default generate the OutEvent enum
• In the following release then just remove NetworkBehaviourEventProcess and event_process.

We'd have to explicitly point that out in our Changelog, but I think it would be more convenient for the majority of our users.

[mxinden]

Change default for event_process to true instead of false. User that would like to keep using it would have to explicitly add #[event_process=false] rather than it being the default

I think there is a confusion here. Today, by default, event_process is false and thereby the derive macro uses the out_event mechanism instead of the NetworkBehaviourEventProcess mechanism. See #2214.

rust-libp2p/swarm-derive/src/lib.rs

Lines 73 to 75 in eaf3f3a

	// Whether or not we require the `NetworkBehaviourEventProcess` trait to be implemented.
	let event_process = {
	let mut event_process = false;

Am I missing something @elenaf9?

[elenaf9]

Change default for event_process to true instead of false. User that would like to keep using it would have to explicitly add #[event_process=false] rather than it being the default

I think there is a confusion here. Today, by default, event_process is false and thereby the derive macro uses the out_event mechanism instead of the NetworkBehaviourEventProcess mechanism. See #2214.

rust-libp2p/swarm-derive/src/lib.rs

Lines 73 to 75 in eaf3f3a

	// Whether or not we require the `NetworkBehaviourEventProcess` trait to be implemented.
	let event_process = {
	let mut event_process = false;

Am I missing something @elenaf9?

You are right, I mixed it up what event_process=false means. But since default event_process=false already means that out_event is used it makes it even easier. So to rephrase my idea above:
Instead of just deprecating NetworkBehaviourEventProcess, in my opinion the first release should already include the changes in the proc macro (and the changes in our examples and other behaviours, so effectively everything from #2751), just that we don't remove the support for event_process = true and NetworkBehaviourEventProcess yet. For users that want to stick with the deprecated event_process nothing would change. The benefit would be that users that do want to switch can then already use the generated enum. Any reason why this would not make sense?

[thomaseizinger]

I think that is a really good idea actually!

[mxinden]

@elenaf9 @thomaseizinger with #2792 merged, would you mind taking another look?

[elenaf9]

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.

[mxinden]

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

rust-libp2p/swarm-derive/tests/test.rs

Lines 354 to 369 in a4110a2

	#[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>();
	}
	}

[mxinden]

Here with NetworkBehaviourEventProcess:

rust-libp2p/swarm-derive/tests/test.rs

Lines 328 to 352 in 2f2b7cb

	#[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>();
	}
	}

[elenaf9]

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:

rust-libp2p/swarm/src/behaviour/toggle.rs

Lines 237 to 239 in a4110a2

	where
	TBehaviour: NetworkBehaviourEventProcess<TEvent>,
	{

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

[elenaf9]

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.

[thomaseizinger]

ACK modulo mine and @elenaf9 's comments :)

[thomaseizinger]

Can we add the changelog entry above so this doesn't come up in the diff?

[mxinden]

I ordered the entries by significance. I don't think users care much about the libp2p-core update. I do think users care about how to upgrade from NetworkBehaviourEventProcess. Does that reasoning make sense @thomaseizinger?

[elenaf9]

One more thing: I think if we are deprecating NetworkBehaviourEventProcess we should also already change our examples & tests to not use it anymore (examples/distributed-key-value-store.rs, examples/chat.rs, ...). Can be done in a separate PR, but imo it should be part of the same release.

[mxinden]

One more thing: I think if we are deprecating NetworkBehaviourEventProcess we should also already change our examples & tests to not use it anymore (examples/distributed-key-value-store.rs, examples/chat.rs, ...). Can be done in a separate PR, but imo it should be part of the same release.

That is a good point. 0f3ef69 does the change.

[mxinden]

As far as I can tell all comments are addressed and everyone approved. Thus I am merging here. Happy to do any follow-ups in case I missed something.

As always, thanks for all the input @elenaf9 and @thomaseizinger!