[RFC][libp2p-swarm] Let network I/O run ahead of NetworkBehaviour polling.

[romanb]

The design of libp2p-swarm is currently such that it tries to progress the underlying Network and the associated NetworkBehaviour in tandem, that is, to poll both with roughly equal frequency. In particular, if the Network has more events (is Ready) but a connection handler is not ready to receive an event emitted by the behaviour, the swarm waits until notified of the handler being ready to make progress again. This PR is a proposal for letting the Network make progress arbitrarily far ahead of the NetworkBehaviour, only subject to back-pressure from the consumer of the swarm events. This is done by continueing the swarm loop instead of returning Poll::Pending in all the cases where a connection handler is currently busy. To be more precise (from the code comments):

// If the pending event cannot be delivered because the connection
// is busy, continue to poll the network. That means the following
// if a single connection (handler) is slow (i.e. does not accept the pending
// event):
//
// 1. All connections are temporarily unable to get new data to send
// from the behaviour (since we only buffer a single pending event).
// This is a problem that still needs to be resolved by providing
// a means for the swarm to exercise connection-specific back-pressure
// on the `NetworkBehaviour`.
//
// 2. New connections continue to get accepted and in general
// network I/O progresses as long as the events emitted by
// the swarm are consumed, i.e. back-pressure on the network
// I/O can only be achieved by the code consuming swarm events.
//
// 3. All connections can continue to receive data and send data
// in the form of events to the `NetworkBehaviour`. All received data
// can be continuously fed into `NetworkBehaviour::inject_event`. It
// may be desirable in the future to allow `inject_event` to signal
// back-pressure via a return value.
//
// 4. As a direct consequence of (3.), `NetworkBehaviour::inject_event` can
// be called any number of times before `NetworkBehaviour::poll` is
// called again (in the absence of a means for `inject_event` to signal
// back-pressure).

Point (3.) implies that instead of letting the connection event buffers in libp2p-core be filled just because a single connection (handler) is slow, the buffering, if there is any, is left to the behaviour (with no direct means for back-pressure, for now). That seems like the better choice because it allows the behaviour and network I/O to make progress (albeit not being poll()ed again until the slow connection handler consumes the pending event).

This obviously has a few consequences on how one writes robust NetworkBehaviours and as mentioned in points (1.) and (3.) is still not free of problems, but may be a step in the right direction.

[twittner]

I agree that this is an improvement, however I worry that many NetworkBehaviour implementations do not handle the internal buffering well.

[tomaka]

I'm quite conflicted about this change.

After this PR, if a node task happens to be slow, rather than freezing the network, all the other node tasks will continue to deliver events, which will be delivered to the NetworkBehaviour and grow an unbounded internal buffer.

In other words, this PR removes the back-pressure on libp2p tasks that comes into play when the Swarm/NetworkBehaviour is slow.

The NetworkBehaviour, since it's not being polled, wouldn't bet capable of delivering events to the user of the Swarm either. In other words, the "network" would still be frozen from the point of view of the outside of the Swarm.

If we indeed do this, I believe that the change to make NetworkBehaviour::inject_event able to indicate back-pressure is non-optional.

[tomaka]

In other words, this PR removes the back-pressure on libp2p tasks that comes into play when the Swarm/NetworkBehaviour is slow.

This would also eliminate the back-pressure on the TCP connections themselves.
Remotes could send us data at a high-speed rate and we would continuously pull data from the socket, even if the rest of the code is too slow to process it.

[romanb]

This would also eliminate the back-pressure on the TCP connections themselves.
Remotes could send us data at a high-speed rate and we would continuously pull data from the socket, even if the rest of the code is too slow to process it.

Yes, absolutely, I share the concerns about the buffers. What I wanted to highlight and primarily seek agreement on in this PR is the fact that the problem of buffering and back-pressure needs to be propagated to the behaviour in a connection-oriented way, because handling it in the Swarm as is done right now makes all progress be subject to the slowest connection.

[arkpar]

Would it be possible to introduce an additional API to Swarm that would try to add an event to the task queue and fail if it is full? This way we can migrate existing code to handle backpressure gradually.
Basically extract this block into a function that accepts NetworkBehaviourAction::NotifyHandler and can be called from Behaviour::poll

[mxinden]

Keep in mind that I have not been involved in this part of the code so far. If this is of no help I would appreciate comments in regards to what I am missing.

As suggested above I am in favor of offloading backpressure to the protocol implementation not having the Swarm buffer any events between NetworkBehaviour and ProtocolsHandler at all.

Protocols control both their NetworkBehaviour as well as their ProtocolsHandler implementation. Thus they can define the level of internal buffering between the two according to their use case (e.g. small buffers for low latency).

The bi-directional communication between a NetworkBehaviour and its ProtocolHandler could leverage the notion of Acks to introduce connection-specific back-pressure.

#### Backpressure: NetworkBehaviour -> ProtocolsHandler

At a given point in time, there can only be a single NotifyHandler event in-flight between a NetworkBehaviour and a specific ProtocolsHandler. I.e. one NotifyHandler event per ProtocolsHandler.

A NetworkBehaviour keeps track for each ProtocolsHandler whether it has a NotifyHandler event in-flight. If not it can emit a NotifyHandler event for that specific ProtocolsHandler. On receiving a NotifyHandler event from a NetworkBehaviour, the Swarm forwards the NotifyHandler event to the corresponding ProtocolsHandler being assured that the ProtocolsHandler accepts the event right away. Once the ProtocolsHandler processed the NotifyHandler event it emits an ProtocolsHandlerEvent::Ack event send to the NetworkBehaviour. On receipt of such event theNetworkBehaviour knows that it can send another NotifyHandler event.

In case a protocol wants to introduce buffers between behaviour and handler they can do so internally without the swarm having to know about it.

#### Backpressure: ProtocolsHandler -> NetworkBehaviour

At a given point in time, there can only be a single ProtocolsHandlerEvent::Custom event in-flight between a ProtocolsHandler and its corresponding NetworkBehaviour.

A ProtocolsHandler keeps track whether it has a ProtocolsHandlerEvent::Custom in-flight. If not it can emit a ProtocolsHandlerEvent::Custom. On receipt of the ProtocolsHandlerEvent::Custom the swarm delivers the event to the corresponding NetworkBehaviour being assured by definition that the NetworkBehaviour can handle the ProtocolsHandlerEvent::Custom right away. Once the NetworkBehaviour processed the ProtocolsHandlerEvent::Custom it emits a NotifyHandler::Ack event to the corresponding ProtocolsHandler which on receive knows that it can send another ProtocolsHandlerEvent::Custom.

Similar like above in case a protocol wants to introduce buffers between handler and behaviour they can do so internally without the swarm having to know about it.

---

The above notion of Acks can already be achieved by and is indirectly required for NetworkBehaviours. The overall comment misses the problem of the pull request, thus deleting it.

[romanb]

Would it be possible to introduce an additional API to Swarm that would try to add an event to the task queue and fail if it is full? This way we can migrate existing code to handle backpressure gradually.
Basically extract this block into a function that accepts NetworkBehaviourAction::NotifyHandler and can be called from Behaviour::poll

I don't think that would work, because it would essentially require access to the underlying Network of the swarm from within NetworkBehaviour::poll, which even if possible would be problematic in terms of encapsulation and the intended inversion of control (i.e. A NetworkBehaviour impl should not be able to make changes to the underlying Network without the Swarm being aware, since the swarm is the mediator). Nevertheless, I'm currently experimenting with a different approach that allows a gradual transition which I will propose for discussion in a new PR.

[tomaka]

primarily seek agreement on in this PR is the fact that the problem of buffering and back-pressure needs to be propagated to the behaviour in a connection-oriented way

While changing inject_event as you propose would make it correct logically-speaking, I'm really not convinced that this is an improvement.
The Swarm would be able to continue running its operations until the next time the NetworkBehaviour wants to send an event to a ProtocolsHandler, but I'm not convinced that this is actually significant.
For example, as I understand it, if the NetworkBehaviour wants to send an event to 50 different connections by returning 50 events in a row, then this PR wouldn't change anything to this (unless we introduce unbounded buffers, which we don't want).

[tomaka]

@mxinden The design of libp2p already permits such thing, not on a connection level but on a substream level. Once a ProtocolsHandler has opened a substream, it can send it to its NetworkBehaviour, which can then directly send and/or receive data on the substream itself. While really not obvious, this is the reason for this comment. The "Manager" is responsible for the communication layer between the NetworkBehaviour and ProtocolsHandler, and must be aware of everything that happens, except specifically for substreams, in order to permit such situations.

[romanb]

@mxinden Sure, you can implement backpressure via an embedded flow-control protocol, but I'm not sure that is the most effective thing to do: Such an embedded flow-control is a form of implementing backpressure by sending friendly messages of the form "Please don't send me any more data until I say so". Notably these flow-control events are sent through (bounded) channels which themselves already offer a means for backpressure by slowing down the speed at which new work (in the form of events) is accepted, leaving at each point the producer of these work items with the consequences of not being able to submit them as fast as it wants to (which usually means further propagation of backpressure).

@tomaka The ability to directly hand over control of negotiated substreams from the connection/protocols handler to the NetworkBehaviour is certainly a useful special case, e.g. typically used with the OneShotHandler, but does still not permit a connection/protocols handler to exert backpressure in a controlled manner with regards to other resource bounds it may wish to implement, like the number of open substreams. Of course one can implement a custom flow-control protocol between the connection / protocols handler and the behaviour to achieve the desired backperssure, but that is quite involved and I am personally convinced that both NetworkBehaviour::inject_event as well as ConnectionHandler::inject_event are in need of easy built-in backpressure facilities. Leaving a connection handler, in the general case where each new event may result in new data being created or propagated further, with the only two options of a) unbounded buffering or b) dropping/discarding events, seems very restrictive.

[romanb]

Closing in favour of #1586.