rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

transports/tcp: use `futures::stream::SelectAll` for driving listener streams

Open elenaf9 opened this issue 3 years ago • 0 comments

Description

With #2652 polling the listeners of a transport became the transport's responsibility. We currently do this in TcpTransport::poll analogous to how it was done before in the swarm ListenerStream: by iterating through the list of TcpListenStreams and sequentially polling each listener for an event, returning the first event from a listener. Instead of doing this manual iteration, we should use futures::stream::SelectAll for polling all TcpListenStreams. This is already used in e.g. the relay ClientTransport and the wasm_ext::ExtTransport.

The following changes would be required:

  • Remove TcpListenerEvent, instead <TcpListenStream as Stream>::Item should directly return a TransportEvent.
  • When a listener was closed, <TcpListenStream as Stream>::poll_next should return Poll::Ready(None) for this listener so that the SelectAll combinator drops the stream
  • Manage the TcpListenStreams in a SelectAll, poll the SelectAll stream in TcpTransport::poll.

Motivation

SelectAll is much more efficient when driving multiple streams since it only polls its inner streams when they generate notifications. Apart from that, it reduces the complexity as we only have to poll the SelectAll stream rather than then manual iteration and polling.

Current Implementation

The TcpListenStreams are pinned in a VecDeque. When the task is woken and TcpTransport::poll called, we iterate through this list by popping the first element, polling it for new events, pushing the listener back onto the list. If the listener returned an TcpListenerEvent we return and TransportEvent for it, else the next listener is polled: https://github.com/libp2p/rust-libp2p/blob/eaf3f3a7fb4aba228ebd15366ab79b64cf37832c/transports/tcp/src/lib.rs#L509-L582

Are you planning to do it yourself in a pull request?

No (or at least not in the near future).

elenaf9 avatar Jul 29 '22 16:07 elenaf9