Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connect to discovered peers concurrently #629

Closed
wants to merge 1 commit into from
Closed

Connect to discovered peers concurrently #629

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion librad/src/net/protocol/accept.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ where
D: futures::Stream<Item = (PeerId, Vec<SocketAddr>)>,
{
disco
.for_each(|(peer, addrs)| {
.for_each_concurrent(10, |(peer, addrs)| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

let state = state.clone();
async move { io::discovered(state, peer, addrs).await }
})
Expand Down