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.

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

feat(kad): Limit in-flight Kademlia queries #3285

Closed
wants to merge 3 commits into from

Conversation

nazar-pc
Copy link
Contributor

Description

Limit number of in-flight Kademlia queries to not exceed configured number of substreams

Notes

I went with a design where each query is first inserted into pending queue and QueryPool will maintain at most 32 queries active at any time, any queries that exceed the limit will remain in pending queue.

In order to guarantee insertion order of pending queries I have introduced an abstraction in second commit.

Since constant restricting inbound streams to 32 is now used for outbound too, I moved it up to the root of the crate and changed name to more generic.

Links to any relevant issues

#3235
#3236

Open Questions

Not entirely sure how to test higher-level behavior here.

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

@nazar-pc nazar-pc changed the title Limit in-flight Kademlia queries feat(kad): Limit in-flight Kademlia queries Dec 27, 2022
@nazar-pc nazar-pc force-pushed the limit-in-flight-kad-queries branch from f3b1666 to 2cf3773 Compare December 27, 2022 12:44
@nazar-pc
Copy link
Contributor Author

I just noticed that performance is severely limited because limit is enforced on Kademlia globally, while it needs to be on per-connection basis, changing to draft for now.

@nazar-pc nazar-pc marked this pull request as draft December 27, 2022 20:23
@dignifiedquire
Copy link
Member

How does this deal with queries being canceled before they have been started? (this is a very relevant use case for my code, where I have run into limits before)

@nazar-pc
Copy link
Contributor Author

It will be removed as if it was cancelled before the first .poll(), so there are no fundamental mechanics changes here.

PR as is does achieve the goal, but with poor performance due to global limit. I'll dig into making limits per-connection instead tomorrow and will update PR once I'm there.

@nazar-pc
Copy link
Contributor Author

Replaced with #3287 that works on handler level instead

@nazar-pc nazar-pc closed this Dec 28, 2022
@nazar-pc nazar-pc deleted the limit-in-flight-kad-queries branch January 23, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants