Multiple connections per peer

[romanb]

Motivation

This is a proposal for #912. Instead of trying to enforce a single connection per peer, which involves quite a bit of additional complexity e.g. to prioritise simultaneously opened connections and can have other undesirable consequences (e.g. paritytech/substrate#4272) besides potentially prohibiting other valid use-cases where multiple connections per peer are desired, this PR makes multiple connections per peer a feature in libp2p-core.

Overview

The concept of a "node" with an implicit 1-1 correspondence to a connection has been replaced with the "first-class" concept of a "connection". The code from src/nodes has moved (with varying degrees of modification) to src/connection and src/network. A HandledNode has become a Connection, a NodeHandler a ConnectionHandler, the CollectionStream was the basis for the new connection::Pool, and so forth.

Conceptually, a Network contains a connection::Pool which in turn internally employs the connection::Manager for handling the background connection::manager::Tasks, one per connection, as before. These are all considered implementation details. On the public API, Peers are managed as before through the Network, except now the API has changed with the shift of focus to (potentially multiple) connections per peer. The NetworkEvents have accordingly also undergone changes.

Backward Compatibility

The API of libp2p-core obviously has changed significantly in detail and semantics, though not so much in the structure (there is still the Network emitting events and Network::peer() for managing peers and almost all existing functions have been preserved, though often with changed semantics due to the nature of permitting multiple connections).

The API of libp2p-swarm should be backward compatible. There is the subtle semantic change that a single NetworkBehaviour can now be associated with many ProtocolsHandler instances for the same peer due to multiple connections, but unless an implementation makes some hard assumptions about such a 1-1 correspondence between peers and ProtocolsHandler instances, which seems unlikely, existing implementations should remain largely unaffected (EDIT: This is incorrect and needs to be addressed. See this comment). libp2p-swarm still does not by itself create multiple connections per peer (i.e. a DialPeer received from a behaviour only results in a connection attempt if the peer is currently disconnected, whereas libp2p-core now also allows additional connect()s for already connected peers), it just may occur as a result of simultaneous dialing. Lastly, inject_connected and inject_disconnected are only called when the first connection is established, respectively when the last connection is lost to a peer, to preserve the existing semantics. inject_replaced is no longer called, since connections are no longer replaced and has thus been deprecated.

Dialing Concurrency

While these changes permit multiple established connections per peer, what I have preserved is the constraint to a single pending outgoing (i.e. "dialing") connection per peer at a time. I think that is still desirable, especially because while dialing is in progress, new addresses to try can be added, e.g. as was and is still done by libp2p-swarm.

Additions

• Instead of just an incoming_limit, there is now also a configurable outgoing limit (pending outgoing connections to any peer) and a configurable limit for the established connections per peer. These limits are enforced by the connection Pool, i.e. the incoming limit is no longer checked on every invocation of Network::poll() to determine if the listeners stream should be polled but rather at the time when they are added to the pool and whenever a pending connection is established. The defaults are no limits for any of these options. Furthermore these are currently hard limits, i.e. there is no throttling based on delays and additional queuing involved.
• There is a NetworkInfo obtained by Network::info() containing some basic connection statistics from the connection pool.
• There is a NetworkConfig for configuring the task executor as well as the connection pool limits.

How to Review

It seems most sensible to first look through the new / adapted modules in src/connection/** and src/connections.rs, followed by src/network/**. and src/network.rs and lastly looking at the actual diffs for libp2p-swarm. A lot of the code and modules in src/connection and src/network will look familiar as it stems from previous src/nodes modules.

Remaining Work

At this point I'm only planning to go over the documentation again,think about new tests and verify with the current polkadot/substrate (e.g. that it indeed addresses paritytech/substrate#4272), but there are no major changes on my backlog other than those that may result from feedback here or from issues encountered in further testing, hence I'm opening this now for review and general feedback.

[romanb]

There is the subtle semantic change that a single NetworkBehaviour can now be associated with many ProtocolsHandler instances for the same peer due to multiple connections, but unless an implementation makes some hard assumptions about such a 1-1 correspondence between peers and ProtocolsHandler instances, which seems unlikely, existing implementations should remain largely unaffected.

After taking a look at libp2p-kad again, I think this is very much incorrect. Indeed, likely any request-reply protocol requires the reply to be sent on the same substream and connection as the original request. That is what e.g. libp2p-kad uses the KademliaRequestIds for which get assigned to incoming requests and are sent back by the behaviour to the handler with the reply.

The least intrusive solution that comes to mind is to let ProtocolsHandler::inject_event (and possibly ConnectionHandler::inject_event as well) signal in the return value whether the given event was accepted / handled. The Swarm can then iterate over the connections to the peer and call inject_event until a connection (handler) accepts it, otherwise drop it.

The alternatives that come to mind involve making a NetworkBehaviour aware of ConnectionIDs, which is likely more intrusive and may be a change that goes well together with removing the ProtocolsHandler altogether later.

[tomaka]

The alternatives that come to mind involve making a NetworkBehaviour aware of ConnectionIDs, which is likely more intrusive and may be a change that goes well together with removing the ProtocolsHandler altogether later.

I would go more in this direction in general.

[romanb]

The alternatives that come to mind involve making a NetworkBehaviour aware of ConnectionIDs, which is likely more intrusive and may be a change that goes well together with removing the ProtocolsHandler altogether later.

I would go more in this direction in general.

While this is clearly where we are heading in general, I'd really like to keep the changes and especially API changes outside of libp2p-core to a minimum in this first iteration.

[mxinden]

Thanks for the detailed write-up for this patch-set.

[romanb]

The alternatives that come to mind involve making a NetworkBehaviour aware of ConnectionIDs, which is likely more intrusive and may be a change that goes well together with removing the ProtocolsHandler altogether later.

I would go more in this direction in general.

While this is clearly where we are heading in general, I'd really like to keep the changes and especially API changes outside of libp2p-core to a minimum in this first iteration.

Turns out it was necessary to do the following two small API changes on libp2p-swarm to allow sending of responses on the same connection:

• Add a connection ID parameter to inject_node_event (and renamed to inject_event for consistency with the rest of the changes).
• Add a connection ID parameter to NetworkBehaviorAction::SendEvent (and renamed to NetworkBehaviourAction::NotifyHandler for consistency with the rest of the changes).

These changes are very straight-forward in terms of code migration. See: ae6bfe9.

[tomaka]

Thanks for doing this!

[twittner]

Why not replacing this racy poll_ready_notify_handler/notify_handler combo with a non-racy version, e.g.

pub fn notify_handler(&mut self, event: I, cx: &mut Context) -> Option<I> {
    if self.poll_ready_notify_task(cx).is_pending() {
        return Some(event)
    }
    let cmd = task::Command::NotifyHandler(event);
    self.notify_task(cmd);
    None
}

[tomaka]

This would be similar to the old AsyncSink enum, and it forces you to potentially store the event somewhere for later when the handler is ready. Meanwhile, with this "racy" API (it's not racy, since it requires &mut), you can generate the event with the guarantee that it is going to be delivered.

[twittner]

We had this discussion in #1373 already.

[romanb]

My preference would be to start with the simpler and safer API as per the suggestion of @twittner and potentially add the poll_ready_x / x combination later as an alternative if needed. That is, we may at the end offer both, just like futures::channel::mpsc::Sender does in form of try_send, start_send and poll_ready.

To elaborate, I'm sure we all understand the motivation and trade-off behind these two styles of API and we just have different priorities w.r.t. what we consider more important. Given that the poll_ready_x / x design is arguably both more "complex" (in that you have to use 2 methods instead of 1) and more error-prone (in that you have to use these two methods in a certain way, subject as well to pitfalls w.r.t multi-threading as already discussed in #1373) I personally prefer to start with just providing try_notify_handler for now. The poll_ready_notify_handler / notify_handler combination could be added later if there is a strong demand or use-case (which as far as I can tell even the Swarm does not currently have).

Does that make sense and sound reasonable?

[romanb]

@tomaka Could you elaborate on the 👎 w.r.t. what aspect you disagree with? Do you insist on offering poll_ready_notify_handler / notify_handler from the start or do you even have objections to offering try_notify_handler?

[tomaka]

This proposition has been refuted in #1373

It hasn't been refuted, I just gave up arguing.

the safer alternative

The definition of "safe" in the context of Rust is "cannot trigger undefined behaviour". In no way misusing this API can trigger any undefined behaviour. The word "safe" is totally irrelevant here.

The worst that can happen is a panic resulting from us misusing the API. There are lots of places in the Rust standard library and in the Rust ecosystem where misusing the API will result in a panic. Polling a Future after it has returned Ready, to give the most prominent example.

Using this API in a correct way is as racy as try_notify_handler is, in the sense that sometimes it will return Pending and sometimes it will return Ready, depending on the timing of the other tasks/threads.

Misuing this API is racy in a similar way as using this API correctly, with the exception that you might get a panic (because you misused the API).

the more dangerous

In order for me to change my point of view, I would like to get a precise definition of what "dangerous" means here for you.

[tomaka]

My issue with try_notify_handler is the fact that there is no way for you to know whether your event will be accepted before generating said event.

Changing the design of Sink between futures 0.1 (where it looked like try_notify_handler) and 0.3 (where it looks like poll_ready_notify_handler/notify_handler) is to me one of the best decisions they made, as it considerably simplified lots of code. I don't want to revert to a futures-0.1-style API.

If panicking really is a big deal, then an API that I think is good is:

impl Manager {
    /// Try to give out a slot for sending an event.
    pub fn poll_ready_notify(&mut self, ...) -> Poll<EventSend> { ... }
}

impl<'a, T> EventSend<'a, T> {
    /// Sends the event, consuming `self`.
    pub fn send_event(self, event: T) { ... }
}

[romanb]

The worst that can happen is a panic resulting from us misusing the API. There are lots of places in the Rust standard library and in the Rust ecosystem where misusing the API will result in a panic. Polling a Future after it has returned Ready, to give the most prominent example.

Note though that the underlying API we're talking about here, futures::channel::mpsc::Sender, never panics (and neither does our current API). Indeed the Sender API says poll_ready should be called before start_send and it consequently still returns an error if the channel is full. It is a bit telling, to me anyway, that they did not go with saying "poll_ready must be called before start_send" combined with panicking due to programmer error on start_send if the channel is full. And they also provide try_send.

Using this API in a correct way is as racy as try_notify_handler is, in the sense that sometimes it will return Pending and sometimes it will return Ready, depending on the timing of the other tasks/threads.

Here we are at a point which I thought was clarified e.g. here. To recap, the crucial difference is in the assumptions that the method that must be called second (e.g. notify_handler) places on the possible errors that can occur and how it deals with them. If it assumes certain errors can never happen because of a requirement that another method must be called first then calls to such two functions which may be interleaved by multiple threads if e.g. Mutexes are used can lead to unexpected behaviour, hence why I already augmented the commentary on notify_handler to say that not only must poll_ready_notify_handler be called first, but also that no other thread must be allowed to interleave between poll_ready_notify_handler and notify_handler. Of course that is easy to satisfy if, as a user, you just call both these &mut methods in sequence on the same thread without e.g. going through a Mutex, but nevertheless it is another thing to watch out for. We may agree to disagree on whether that justifies calling an API "racy", but in any case the futures::channel::mpsc::Sender API is actually not racy, whatever definition you choose, whereas the API provided by us, currently, is considered racy by myself and @twittner because it may silently (well, now it logs at least) ignore errors due to the channel being full when calling notify_handler.

I have added try_notify_handler in a new commit in the hope that it is uncontroversial since it follows the example of the futures::channel::mpsc::Sender API, leaving the other functions in place. Is that acceptable?

[twittner]

This proposition has been refuted in #1373

It hasn't been refuted, I just gave up arguing.

I would love to hear a new argument then. If "it's not racy, since it requires &mut" would be true why would we even have this discussion? Since the outcome of concurrent calls to poll_ready_notify_handler and notify_handler depends on relative timings there clearly is a race condition here that the individual &muts in each of these methods can not prevent.

the safer alternative

The definition of "safe" in the context of Rust is "cannot trigger undefined behaviour". In no way misusing this API can trigger any undefined behaviour. The word "safe" is totally irrelevant here.

[...]

the more dangerous

In order for me to change my point of view, I would like to get a precise definition of what "dangerous" means here for you.

Rust does not have a monopoly on the word "safe" and since you were asking for a definiton of "dangerous", I am happy to elaborate: Assuming there are undesirable program states, e.g. a panic, an API is safe w.r.t. those states if it can by construction guarantee that those states can never occur. Correspondingly an API is dangerous w.r.t. those states if it can not give those guarantees.

Misuing this API is racy in a similar way as using this API correctly, with the exception that you might get a panic (because you misused the API).

This "exception" being an undesirable program state and the fact that the API can not guarantee its own correct use but allows for degrees of freedom that can lead to such a state makes it more dangerous in my book.

My issue with try_notify_handler is the fact that there is no way for you to know whether your event will be accepted before generating said event.

We could combine both worlds by keeping poll_ready_notify_handler as an indication that most likely notify_handler will be able to accept the event, but in case it can not (because of API misuse) we can at least return the event:

pub fn poll_ready_notify_handler(&mut self, cx: &mut Context) -> Poll<()>;

pub fn notify_handler(&mut self, event: I, cx: &mut Context) -> Option<I>;

[romanb]

We could combine both worlds by keeping poll_ready_notify_handler as an indication that most likely notify_handler will be able to accept the event, but in case it can not (because of API misuse) we can at least return the event:

pub fn poll_ready_notify_handler(&mut self, cx: &mut Context) -> Poll<()>;

pub fn notify_handler(&mut self, event: I, cx: &mut Context) -> Option<I>;

I would be ok with that as well, and it would make try_notify_handler superfluous (notify_handler would then internally use Sender::try_send which is used by Sender::start_send internally anyway).

[tomaka]

The test is repeatedly failing.

[romanb]

The test is repeatedly failing.

Which test do you mean? They all look green right now. Relatedly, my proposal for the substrate integration is now also ready for review (but obviously not for merging): paritytech/substrate#5066.

[tomaka]

Which test do you mean?

Hah, I restarted the tests once again right after posting my comment, and now it's green.

[romanb]

Which test do you mean?

Hah, I restarted the tests once again right after posting my comment, and now it's green.

I see, I can look at the ipfs-kad example again, but I would probably as a first guess blame IPFS nodes. It may help if we increase the acceptable maximum payload size of Kademlia requests / responses from 4KiB to 8KiB. I remember often seeing failures due to that size limit, sometimes even from the first node. There may be other issues as well, of course.

[tomaka]

I would probably as a first guess blame IPFS nodes.

That would also be my guess.