Backpressure and the ConnectionHandler interface

[thomaseizinger]

We have a tracking issue around backpressure in rust-libp2p: #3078. Recently, a discussion came up where lack of backpressure actually causes nodes to crash because they run out of memory: #4572. In this particular case, we'd need backpressure from the Behaviour to the ConnectionHandlers.

If we were to adapt this interface, we'd have to add several new functions to NetworkBehaviour and ConnectionHandler. Something like NetworkBehaviour::poll_next_message to send a new message to a ConnectionHandler and we'd need a poll_ready-like function on ConnectionHandler where it can reports its current state.

In light of #2863, I would like to suggest a somewhat radical idea of how we could easily fix this:

• Entirely remove the built-in messaging mechanism between NetworkBehaviour and ConnectionHandler and with it, the ConnectionHandler::{To,From}Behaviour associated types
• Introduce a generic utility called mpmc::Channels (naming subject to bike-shedding)

If a protocol needs to communicate between NetworkBehaviour and ConnectionHandler, the NetworkBehaviour would own an instance of mpmc::Channels. When constructing a new ConnectionHandler, the behaviour registers a new key within mpmc::Channels for the new connection.

• mpmc::Channels would internally retain a mapping of keys => channels
• Calling mpmc::Channels::poll reads messages from all connected channels and "tags" them with the corresponding key (in our case, ConnectionId)
• mpmc::Channels would offer several functions for sending messages:
  • poll_all_ready(&mut self, cx: &mut Context): Checks if all connected channels are ready to receive an item.
  • send_to_all(&mut self, msg: M) where M: Clone -> Result<()>: Sends a message into all channels. Fails if any channel is not ready. Should be paired with poll_all_ready.
  • poll_ready(&mut self, key: K, cx: &mut Context)
  • send_to(&mut self, key: K, msg: M) -> Result<()>
• When registering a new channel for a given key, one is given a Channel which implements Sink + Stream. The NetworkBehaviour hands that to the ConnectionHandler.

I see several advantages of this scheme:

• We can support backpressure between NetworkBehaviour and ConnectionHandler without extending any APIs.
• The ConnectionHandler interface becomes simpler because the associated types go away.
• libp2p-swarm will be simpler because I think it should be possible to implement the above in a standalone crate.
• The enums ToSwarm and ConnetionHandlerEvent will become simpler.
• We can easily add more utility functions on mpmc::Channels that support advanced usecases like:
  • sending to all handlers
  • sending to any handler that is ready
  • sending to the handler with the most capacity

The only downside is that users have to add a struct to their custom behaviours and manually poll something within the provided poll functions. However, I'd consider that to be pretty simple scaffholding that is easy to apply once understood. We already expect users to understand poll-like interfaces when they implement a custom NetworkBehaviour so I'd say this is fine.

@libp2p/rust-libp2p-maintainers Please voice your opinions.

[nazar-pc]

I'm wondering if mpmc::Channel would have any kind of buffering internally.

Because if it does then it may not cover one of the use cases where action might be initiated, but later cancelled. For example there could be two ways to retrieve something racing with each other and whichever succeeds first should result in the other one being aborted/cancelled.

[thomaseizinger]

We also currently use channels behind the scenes with a buffer. There is no guarantee that a certain message will reach the ConnectionHandler.

I think the easiest way to implement such a race would be to tag all requests with a certain identifier and whichever one completes first sets some state in the ConnectionHandler. Upon receiving a new message, it can then decide based on this state what to do. When one of them completes, it can also discard existing work (assuming it is f.e. polling Futures itself and not using the OutboundUpgrade mechanism).

[nazar-pc]

I'm wondering what external API would look like. It would be really unfortunate from developer experience to deal with all ofthose identifiers. It is much better to just call two async functions, race returned futures in any of the number of ways (for example with select! {} macro) and once one of them completes just drop the other one.

We have already implemented some portions of necessary backpressure in our app and we used resizable semaphores, then just dropping future with semaphore that didn't get a permit yet cancelles it. Of course it doesn't propagate as deep as we want it to, but still.

[thomaseizinger]

I'm wondering what external API would look like. It would be really unfortunate from developer experience to deal with all ofthose identifiers. It is much better to just call two async functions, race returned futures in any of the number of ways (for example with select! {} macro) and once one of them completes just drop the other one.

Yeah that makes a lot of sense. @mxinden and I have been discussing a more async-friendly interface for network behaviours that is based on "controls", i.e. cheap-clonable handles with async functions that directly return their result, send messages into an internal event-loop and the response back using a one-shot or something similar. Such an interface would probably allow cancellation easily by detecting whether the oneshot receiver has been dropped. We are currently finalising a new, more user-friendly SwarmBuilder. Experimenting with such a control interface might come after that.

Collaboration is always welcome!

[mxinden]

I think this is an improvement worth exploring. Though instead of designing a generic mechanism right away, I suggest solving #4667 or #3710 first, and from there try to find a generic mechanism.