Connect to discovered peers concurrently #629
Conversation
Before, connecting to a discovered peer would block the connecting to the peer discovered next. This would cause particular problems when we tried to connect to a peer that isn’t reachable and the connection would result in a timeout. Signed-off-by: Thomas Scholtes <[email protected]>
| @@ -28,7 +28,7 @@ where | |||
| D: futures::Stream<Item = (PeerId, Vec<SocketAddr>)>, | |||
| { | |||
| disco | |||
| .for_each(|(peer, addrs)| { | |||
| .for_each_concurrent(10, |(peer, addrs)| { | |||
There was a problem hiding this comment.
I see how this is useful.
However, there is a catch: when we discover a new peer, we'll try to add it to the active set. Which may cause already-active peers to get demoted.
This is problematic when the disco stream is not coming from a static (bounded) set of bootstrap nodes, but for example from some periodic (m)DNS query, or some datasource which remembers a list of seen peers which is larger than the active set: the active set will constantly get replaced, causing a lot of churn. It will also converge the node to only be connected to the peers which are discovered via this stream, not the membership shuffle.
I haven't been able to come up with a good solution to this, but now that you're raising it: maybe the disco future could just go into a pending state while the membership active count (available through state.membership.view_stats().0) is equal to net::protocol::membership::Params::max_active (not currently available to this function)? When it wakes up, and the count is smaller, it could pop max_active - active from the stream and try to connect to those concurrently.
There was a problem hiding this comment.
It will also converge the node to only be connected to the peers which are discovered via this stream, not the membership shuffle.
To elaborate on why this is bad: specifically mDNS could yield many peers on the local network, so we'd converge to only be connected to the local network. Which is clearly not what we want.
There was a problem hiding this comment.
Would it make sense then to handle the connection (io::connection::connect()) concurrently but the membership sequentially? In this case unreachable peers would not block membership rpc with other peers.
The motivation for this PR is seed node selection in Upstream. Concretely, we just want to tell link to connect to a set of nodes. As an alternative to what I described above we could also provide a better API to achieve the Upstream use case. We could have a function that tells the local peer to connect to another remote node or we could change the discovery stream to impl Stream<Item = Vec<PeerId, Vec<SocketAddr>>>. (I’d prefer the former).
|
Would it make sense then to handle the connection
(`io::connection::connect()`) concurrently
I was under the impression that `select_ok` is effectively concurrent, but that
could be wrong.
We could have a function that tells the local peer to connect to another
remote node or we could change the discovery stream to `impl Stream<Item =
Vec<PeerId, Vec<SocketAddr>>>`. (I’d prefer the former).
With the current API, it is fully the responsibility of the caller to ensure
discovered peers are yielded in the right order and frequency. I tried to
explain why this is not ideal, but what you propose is not an improvement -- it
just gives the caller more liberty to interfere with the membership protocol,
where the problem is that it should be up to the protocol to decide when resort
to external discovery.
The issue has been discussed multiple times, but no one came forward with a
solution. I just tried to give one. If you don't want to solve this, that's
okay. We can create a ticket, and eventually someone will pick it up. #211
covers the issue as well.
|
Sorry that I didn’t make this clear but I’m wondering whether to provide an API for the case where the user (e.g. Upstream) wants the peer to connect to a specific remote now. This API would be distinct from the discovery stream (and mDNS) which would only pull a new peer if an active slot is available. With this new API it would of course be the responsibility of the caller to ensure that this does not mess things up.
I’ll update the PR incorporating your suggestion. That should shed more light on it. |
|
Sorry that I didn’t make this clear but I’m wondering whether to
provide an API for the case where the user (e.g. Upstream) wants the
peer to connect to a specific remote now.
I understood that, but I don't think it's a good idea. There is no reason the
user should ever interfere with gossip membership, and if that's the case now,
then because the disco API wasn't fleshed out fully, as described above.
Note that one can always establish connections in other ways, for example by
initiating a git fetch directly, or via the interrogation API. This doesn't
affect membership at all.
But we also want to try to connect to multiple peers concurrently to avoid an
unreachable peer blocking other connections.
Sure yes, it is also conceivable to pop more addresses off the stream than would
strictly be needed. The first one to succeed would then be passed to
`discovered`, and the rest dropped and eventually be GC'ed.
The property of the disco stream we _actually_ want is that it is infinite. That
is, the static disco would cycle the list. That is not possible now, because
we're not pulling only when needed. If we did, however, it would be possible to
have the network stack reconnect without having to drop and recreate it.
This would still be a problem if we only pull from the discovery stream when
there’s a free slot.
Why is it a problem?
But maybe sequentially connecting is also okay if we set a timeout for the
connection.
There is a timeout, it's just much larger than one is used to from TCP -- it
just keeps sending the initial frame until the packet loss detector kicks in. It
is surely worthwhile to try and tune this, but that will require some actual
data.
|
|
Can we close this in favour of #723? |
Before, connecting to a discovered peer would block the connecting to the peer discovered next. This would cause particular problems when we tried to connect to a peer that isn’t reachable and the connection would result in a timeout.