swarm: change Stream to return all SwarmEvents

[elenaf9]

PR for the suggested changes from @mxinden in #1876.

Change Stream implementation for ExpandedSwarm to return all SwarmEvents instead of only the network behaviour 's events.
Remove redundant next_event() method.
Rename next() method to behaviour_next() (maybe find a better name for it?). I'd suggest to keep this method for awaiting the next network behaviour event, because it is more convenient than using StreamExt::filter()/StreamExt::filter_map().
Adjust examples to the change.

[thomaseizinger]

Unfortunately, rust-libp2p doesn't use rustfmt, see #1966.

Do you mind removing the formatting diffs? Makes it quite hard to review.

[mxinden]

Thanks @elenaf9 for tackling this!

When using libraries that offer async methods only, I find it difficult to judge whether the Futures returned by these async methods are cancellation correct (i.e. state lesss or safe to poll once and then drop on Poll::Pending). A library can make this explicit by offering both a poll style and async method. Long story short, what do you think of offering both poll_next_behaviour_event and next_behaviour_event along the lines of:

/// Convenience function only returning events produced by the [`NetworkBehaviour`].
///
/// For `async` version see [`ExpandedSwarm::next_behaviour_event`].
///
/// For all events via [`SwarmEvent`] see [`Stream`] implementation on [`ExpandedSwarm`].
fn poll_next_behaviour_event(
    mut self: Pin<&mut Self>,
    cx: &mut Context<'_>,
) -> Poll<TBehaviour::OutEvent> {
    loop {
        let event = futures::ready!(ExpandedSwarm::poll_next_event(Pin::new(self), cx));
        if let SwarmEvent::Behaviour(event) = event {
            return Poll::Ready(event);
        }
    }
}

/// Convenience function only returning events produced by the [`NetworkBehaviour`].
///
/// For `poll` version see [`ExpandedSwarm::poll_next_behaviour_event`].
///
/// For all events via [`SwarmEvent`] see [`Stream`] implementation on [`ExpandedSwarm`].
pub async fn next_behaviour_event(&mut self) -> TBehaviour::OutEvent {
    future::poll_fn(self.poll_next_behaviour_event)
}

[elenaf9]

Long story short, what do you think of offering both poll_next_behaviour_event and next_behaviour_event

I agree that generally the Poll style should be provided, but wouldn't Stream::poll_next_unpin() (and matching on SwarmEvent::Behaviour) also implement the necessary logic here? I think (please correct me if I am wrong) this is more a question of convenience vs complexity, and how I understood you initial comment on #1876, the goal is to reduce the complexity. Or do you think it is better to add it anyways @mxinden?

[mxinden]

I think (please correct me if I am wrong) this is more a question of convenience vs complexity, and how I understood you initial comment on #1876, the goal is to reduce the complexity. Or do you think it is better to add it anyways @mxinden?

I have no need for poll_next_behaviour_event nor next_behaviour_event, in any of my projects using rust-libp2p, thus I would be in favor of removing both, for the sake of reducing complexity.

That said, in case people do need either poll_next_behaviour_event or next_behaviour_event, I am happy to include them, but would advocate for including both, for the sake of encoding the cancellation safety on the type level. I think including both is worth the additional complexity.

---

On a related note, by removing the original next method, we no-longer offer an infinite-stream at the type level (see rust-lang/futures-rs#1857 for related discussions). In other words, users now need to handle the case where Stream::poll_next returns None, even though None is never returned by <ExpandedSwarm as Stream>::poll_next.

I think the loss in type safety by removing ExpandedSwarm::next in favor of <ExpandedSwarm as Stream>::poll_next is worth the reduced complexity and the additional compatibility with the whole StreamExt method suite.

Everyone, please speak up in case you think differently.

[thomaseizinger]

I have no need for poll_next_behaviour_event nor next_behaviour_event, in any of my projects using rust-libp2p, thus I would be in favor of removing both, for the sake of reducing complexity.

I share the same view and would argue that the provided convenience is almost negligible given that in any application, there will hopefully only be a single place where the swarm is being polled. It is not like swarm.next_behaviour_event is such a commonly used method that it would warrant making it as convenient as possible.

On a related note, by removing the original next method, we no-longer offer an infinite-stream at the type level (see rust-lang/futures-rs#1857 for related discussions). In other words, users now need to handle the case where Stream::poll_next returns None, even though None is never returned by ::poll_next.

That is a good point but same as above, I would argue that the convenience hit is minimal given that it probably only occurs within a single place in any application.

One could even argue that providing only a single way of doing things is better. For example, by committing fully to the Stream interface, it is easy to write extension traits for Stream that are specific to rust-libp2p if one wanted to make certain handling of events more ergonomic (I am currently working an extension trait that makes setting up a Swarm for testing purposes easier.)

[elenaf9]

I think the loss in type safety by removing ExpandedSwarm::next in favor of ::poll_next is worth the reduced complexity and the additional compatibility with the whole StreamExt method suite.

I completely agree with this, but maybe we should add to the Stream implementation some comment/ docs on this stating that it will never return Poll::Ready(None) and thus it can be guaranteed that StreamExt::select_next_some will never panic?
Maybe something along the lines of:

Stream of events that are happening in the `Swarm`. 
Includes events from the `NetworkBehaviour` but also events about the connections status and listeners.

This stream is infinite and it is guarenteed that `Stream::poll_next will never return `Poll::Ready(None)`.

Please feel free to suggest some rephrasing or different terminology.

I have no need for poll_next_behaviour_event nor next_behaviour_event, in any of my projects using rust-libp2p, thus I would be in favor of removing both, for the sake of reducing complexity.

I share the same view and would argue that the provided convenience is almost negligible given that in any application, there will hopefully only be a single place where the swarm is being polled. It is not like swarm.next_behaviour_event is such a commonly used method that it would warrant making it as convenient as possible.

Considering your arguments, I agree that both poll_next_behaviour_event and next_behaviour_event should be removed. While I originally suggested to keep next_behaviour_event because I though it might be commonly used (as it was also often used in the examples), I understand that in this case poll_next_behaviour_event should also be implemented. After re-thinking this, I also favor reduced complexity and having a single way of doing things, instead of multiple methods where it might not be clear if/how they are related.

[thomaseizinger]

LGTM, thank you!

[mxinden]

Great. Thanks for the follow up work!

Would you mind including the diff below, adding a comment to ExpandedSwarm and a changelog entry?

Other than that, this is ready to merge from my side.

diff --git a/swarm/CHANGELOG.md b/swarm/CHANGELOG.md
index 422220fe..29019da5 100644
--- a/swarm/CHANGELOG.md
+++ b/swarm/CHANGELOG.md
@@ -2,6 +2,21 @@
 
 - Update dependencies.
 
+- Drive `ExpandedSwarm` via `Stream` trait only.
+
+  - Change `Stream` implementation of `ExpandedSwarm` to return all
+    `SwarmEvents` instead of only the `NetworkBehaviour`'s events.
+
+  - Remove `ExpandedSwarm::next_event`. Users can use `<ExpandedSwarm as
+    StreamExt>::next` instead.
+
+  - Remove `ExpandedSwarm::next`. Users can use `<ExpandedSwarm as
+    StreamExt>::filter_map` instead.
+
+  See [PR 2100] for details.
+
+[PR 2100]: https://github.com/libp2p/rust-libp2p/pull/2100
+
 # 0.29.0 [2021-04-13]
 
 - Remove `Deref` and `DerefMut` implementations previously dereferencing to the
diff --git a/swarm/src/lib.rs b/swarm/src/lib.rs
index 28ea83ee..af0936bd 100644
--- a/swarm/src/lib.rs
+++ b/swarm/src/lib.rs
@@ -256,6 +256,9 @@ pub enum SwarmEvent<TBvEv, THandleErr> {
 }
 
 /// Contains the state of the network, plus the way it should behave.
+///
+/// Note: Needs to be polled via [`<ExpandedSwarm as Stream>`] in order to make
+/// progress.
 pub struct ExpandedSwarm<TBehaviour, TInEvent, TOutEvent, THandler>
 where
     THandler: IntoProtocolsHandler,

[mxinden]

Thanks @elenaf9! I will merge later today.