Refactor UdpMuxNewAddr to be lock-less
#1
Conversation
The general idea is this: We use `flume` channels between the actual `UdpMuxNewAddr` instance and a set of "handles", one per async-trait that we need to satisfy. There is only one instance of `UdpMuxNewAddr`, meaning we can access it via `&mut self` and drop all locks within it. In the poll function, we can then decide, which events to prioritise over others. Most importantly, all channels between handles and the actual instance are "rendezvous channels", meaning, sending will block until we actually read the item out of the channel. This allows us to enforce backpressure from the poll function all the way to all handles and into the task that is interacting with the handle.
There was a problem hiding this comment.
I am in favor of this design, namely a single UdpMuxNewAddr, connected to all remaining components via channels.
If I am not mistaken with the current state of this pull request UdpMuxNewAddr would be driven by WebRTCListenStream, which is driven by Transport::poll, which is driven by the main libp2p event loop, making the whole execution in some sense single threaded. I don't think this needs to be tackled right away. We can fix this similar to QUIC via a single task per UdpMuxNewAddr or a single task per WebRTCTransport.
| @@ -0,0 +1,49 @@ | |||
| use futures::channel::mpsc; | |||
There was a problem hiding this comment.
Missing license header. Feel free to ignore.
There was a problem hiding this comment.
Can I? I'd love to get rid of those 😅
There was a problem hiding this comment.
Haha, I meant "Feel free to ignore [for now]".
I think we should be consistent in this pull request.
I am fine getting rid of them in general, though I would need to check back with some legal experts first.
|
On the meta level, thanks @thomaseizinger for writing this PoC. Especially the recent version reads very cleanly. |
|
Overall I am in favor of this architectural change. I hope for this to resolve issues like libp2p#2622 (comment) and libp2p#2622 (comment). @melekes do I understand correctly that you still favor the |
thomaseizinger
changed the title
UdpMuxNewAddr to be lock-less
This is to ensure backpressure. To not loose drop any packets, we only read from the socket in case we are not currently writing a packet.
Instead of buffering multiple items, we only ever buffer one. To work off this queue as quickly as possible, we put it at the top of the loop which allows us to reduce some code duplication by writing directly to the buffer in case we popped an item off the queue and going back to the start of the loop which will trigger a different code branch.
|
Thanks @thomaseizinger for the refactoring and thanks @melekes for merging. |
TBD