Handling multiple in-flight requests

[carver]

When sending multiple requests, the current design queues them, waiting for a response to the first request before sending the next one:

discv5/src/handler/mod.rs

Lines 468 to 481 in a9ded04

	if self.active_requests.get(&node_address).is_some()
	|| self.active_challenges.get(&node_address).is_some()
	{
	trace!("Request queued for node: {}", node_address);
	self.pending_requests
	.entry(node_address)
	.or_insert_with(Vec::new)
	.push(PendingRequest {
	contact,
	request_id,
	request,
	});
	return Ok(());
	}

If a client has a 100ms roundtrip time with a peer, that puts a hard upper limit of 10 messages per second with that peer. That limits the "bandwidth" to less than 12.8kB/s (102 kb/s).

There's nothing apparent in the protocol spec that requires 1 message at a time. Including the request ID suggests that the spec intends to support multiple in-flight requests.

What are maintainers' thoughts on reviewing a PR that would support multiple in-flight requests?

[fjl]

It is certainly possible to run concurrent requests, but please note that it will make session handling much more complicated. Remember that in discv5, it is assumed the session may be dropped at any moment. If the node has multiple requests in flight with a peer, and the peer answers with a challenge (WHOAREYOU), all pending requests would need to be replayed. It is much easier to implement the handshake flow when there is the guarantee that only a single request is pending for each peer.

[AgeManning]

Yep. I think in its original design it allowed for multiple con-current in-flight requests. But handling all the edge cases got pretty unwieldy and there were significant code simplifications we can make by having a single one per-peer.

The other issue you may run into, is that we also need a way to determine whether a peer is still alive/online. There is a configuration parameter that allows you to retry a request if it fails. If we have many outgoing requests at a time and some fail, should we consider the connection failed?

At the moment I think the default is that if we get a request time out, we just consider the peer offline and move on. This is helpful for discovery queries where we are asking many different peers lots of things at once, rather than a single peer a lot of things.

[njgheorghita]

Took a stab at implementing this in #200 - would love to hear some feedback. Did I take the right approach? Did I miss any edge cases?

If we have many outgoing requests at a time and some fail, should we consider the connection failed?

I took the approach of "yes". As well as replaying all active requests if a new challenge is received.

I had very successful results using this inside trin to facilitate our utp transfers, which were really struggling beforehand when concurrent requests were not allowed.

[ackintosh]

It has been implemented by #200 and was released in v0.4.0. 🎉