feat(kad): introduce AsyncBehaviour

[stormshield-frb]

Description

The more things our software is doing, the more it is complicated to track Kademlia queries. We find ourselves needing to have distinct HashMap to keep track of every queries we are doing so we can link them back to where and why there were triggered and we needed them. This began to be very messy and that is why we have implemented a wrapper of the kad::Behaviour with the goal of simplifying the tracking of Kademlia queries.

This wrapper is pretty simple and has one method for each of the possible Kademlia queries (bootstrap, get_closest_peers, etc) respectively suffixed _async (bootstrap_async, get_closest_peers_async, etc). Those methods, instead of returning a QueryId, return a typed UnboundedQueryResultReceiver that will receive every Event::OutboundQueryProgressed corresponding to the QueryId. The only purpose of this wrapper is to map an OutboundQueryProgressed to the corresponding sender and send it.

Doing so, it is very easy for the developer to track his queries and keep a correct state in his application specific code.

Notes & open questions

I have chosen to use UnboundedChannel considering that the data transmitted is already in memory since this we obtain it when receiving an OutboundQueryProgressed. That is why I don't think there is a particular risk of running out of memory. If there is, I can rework my wrapper to work with bounded channels.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[drHuangMHT]

I think it's OK to drop some messages though, they are not that important.
I like the idea of directing query result to the query-er but the event system is designed in such a way so that everyone can listen on any event emitted by anyone. However I do encounter times when swarm events overwhelm consumers and that forced me to implement backpressure so this is to-be-discussed.

[jxs]

Hi François, thanks for starting this! I think this is a great idea for kademlia to offer async await friendly primitives, left some notes to help move this forward

[jxs]

What do you think of following the design introduced with stream, i.e. introducing a clonable Control
handle which implements the methods, that way we don't need to keep a reference to the Swarm to call the methods.

[stormshield-frb]

Yeah it would probably be great !!

I'm thinking about it but I really don't see how I could implement it since I need to capture the events emitted by the kad::Behaviour. Do you have an idea ?

[stormshield-frb]

@jxs , I've thought a little bit about this. The only way I see to make a Control work for the kad::Behaviour is to have the entire Behaviour behind an Arc<Mutex<>>. I don't know if it is a good or bad idea.

Will the "control" semantic become something standard for all the behaviours ? If so why not. But if not, I don't know how it will be understood by the end user to have some behaviours using a control and others not.

I'd really like to also have your opinion on this @guillaumemichel when you have the time.

[guillaumemichel]

I am discovering how Control works and I think it would make sense in this case. What would be the arguments against using it?

[elenaf9]

@jxs , I've thought a little bit about this. The only way I see to make a Control work for the kad::Behaviour is to have the entire Behaviour behind an Arc<Mutex<>>. I don't know if it is a good or bad idea.

I don't think you need an Arc/ Mutex.
You could split the current AsyncBehavior logic into two parts: the Control part and the NetworkBehavior part, that communicate through an additional mpsc channel (the "command-channel").

• Control would have all the async variants of the kademlia behavior (get_closest_peers_async, get_providers_async, etc.).
  There, it would create the mpsc channel for the results, and then send the a tuple (Command::GetProviders {..}, mpsc::Sender<AsyncQueryResult>) through the "command-channel" to the AsyncBehavior.
• AsyncBehavior would wrap the kademlia behavior and impl NetworkBehavior. In its poll_next loop it would poll the receiving side of the command-channel, and handle the incoming command by calling the matching function on the wrapped kademlia behavior and storing the Sender in the query_result_senders hashmap. poll_next would also still have the already existing logic for intercepting ToSwarm::GenerateEvent events from the inner behavior and forwarding the results.

Does that make sense? @jxs was that roughly what you had in mind?

[stormshield-frb]

@jxs like I mentioned in this PR description (in section Notes and open questions), I have used unbounded channels thinking that this could not cause a memory issue more than what there is already since all the sent events are already in memory. Still, clippy does not agree with me and the CI fails. What do you prefer ? I put an allow(clippy::disallowed_method) or I try to use bounded channels ?

I really don't have an opinion on this.

[dariusc93]

@jxs like I mentioned in this PR description (in section Notes and open questions), I have used unbounded channels thinking that this could not cause a memory issue more than what there is already since all the sent events are already in memory. Still, clippy does not agree with me and the CI fails. What do you prefer ? I put an allow(clippy::disallowed_method) or I try to use bounded channels ?

I really don't have an opinion on this.

It would be preferred to use bounded channels so we dont introduce that vector.

[mergify]

This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏

[guillaumemichel]

Sorry for the late comment, I like the idea of having async updates from the query in an easier manner. I would also prefer to avoid unbounded channels

[elenaf9]

The query could also have been triggered through the non-async methods of the inner kad Behavior that we deref to, right?

[elenaf9]

@jxs , I've thought a little bit about this. The only way I see to make a Control work for the kad::Behaviour is to have the entire Behaviour behind an Arc<Mutex<>>. I don't know if it is a good or bad idea.

I don't think you need an Arc/ Mutex.
You could split the current AsyncBehavior logic into two parts: the Control part and the NetworkBehavior part, that communicate through an additional mpsc channel (the "command-channel").

• Control would have all the async variants of the kademlia behavior (get_closest_peers_async, get_providers_async, etc.).
  There, it would create the mpsc channel for the results, and then send the a tuple (Command::GetProviders {..}, mpsc::Sender<AsyncQueryResult>) through the "command-channel" to the AsyncBehavior.
• AsyncBehavior would wrap the kademlia behavior and impl NetworkBehavior. In its poll_next loop it would poll the receiving side of the command-channel, and handle the incoming command by calling the matching function on the wrapped kademlia behavior and storing the Sender in the query_result_senders hashmap. poll_next would also still have the already existing logic for intercepting ToSwarm::GenerateEvent events from the inner behavior and forwarding the results.

Does that make sense? @jxs was that roughly what you had in mind?