refactor(kad): use oneshots and async-await for outbound streams

[thomaseizinger]

Description

This refactoring addresses several aspects of the current handler implementation:

• Remove the manual state machine for outbound streams in favor of using async-await.
• Use oneshots to track the number of requested outbound streams
• Use futures_bounded::FuturesMap to track the execution of a stream, thus applying a timeout to the entire request.

Resolves: #3130.
Related: #3268.
Related: #4510.

Notes & open questions

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

[mxinden]

Overall this is great!

🎉

I don't find the pattern of request-stream-and-pass-it-through-oneshot-to-the-already-started-future intuitive. Given the use of a oneshot, I would expect some level of concurrency, whereas this is strictly sequential.

Also, long term, I would like to have backpressure between NetworkBehaviour and ConnectionHandler. The ConnectionHandler should only accept a new request from the NetworkBehaviour, when the previous request has a new channel and is being sent. The current pattern does not suite this. Though one could argue that this is easy to refactor, once we introduce backpressure.

I see the problem, and appreciate the work here and on #4834. I suggest we discuss this in tomorrow's open maintainers call. In case we can not come up with an alternative, I suggest we go with your proposal here.

[thomaseizinger]

Also, long term, I would like to have backpressure between NetworkBehaviour and ConnectionHandler. The ConnectionHandler should only accept a new request from the NetworkBehaviour, when the previous request has a new channel and is being sent. The current pattern does not suite this.

Why not? At any point, we know how many streams are currently being opened by checking how many Senders we have. A backpressure-aware API can make use of this information to slow down incoming requests.

I don't find the pattern of request-stream-and-pass-it-through-oneshot-to-the-already-started-future intuitive. Given the use of a oneshot, I would expect some level of concurrency, whereas this is strictly sequential.

I am not sure I entirely follow your reasoning here. To me, a oneshot is useful if I want to suspend a Future on some missing state at the time of constructing it. It is simply a level of indirection.

The oneshot is only necessary because our current APIs don't support expressing the sequential nature of opening a stream and doing something with it using async-await. In an ideal world, a ConnectionHandler would have a way of obtaining a Future that provides it with a Stream such that it can directly compose the entire protocol onto it.

In fact, the more I think about this, the more obvious and elegant it appears to me because it simplifies so much of the state management in ConnectionHandlers.

[thomaseizinger]

In case we can not come up with an alternative, I suggest we go with your proposal here.

I am open to alternatives. I couldn't think of any that we can ship as easily as this because many APIs would have to change drastically.

[thomaseizinger]

@mxinden I've renamed the function. Note that, due to our magic 32, we still have a potentially unbounded buffer here because messages can pile up in pending_messages.

I'd suggest deferring this to a later point though. For now, I'd like to push the use of oneshots for outbound streams forward. Together with a tutorial on how to write your own ConnectionHandler, I see that as the solution to finialising #3268.

[mxinden]

Ready to merge apart from the ongoing discussion above.