rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

feat(kad): introduce AsyncBehaviour

Open stormshield-frb opened this issue 1 year ago • 5 comments

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

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

stormshield-frb avatar Apr 05 '24 12:04 stormshield-frb

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.

drHuangMHT avatar Apr 05 '24 15:04 drHuangMHT

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

stormshield-frb avatar Apr 09 '24 15:04 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.

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

dariusc93 avatar Apr 15 '24 23:04 dariusc93

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

mergify[bot] avatar Apr 19 '24 07:04 mergify[bot]

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

guillaumemichel avatar Sep 06 '24 15:09 guillaumemichel