refactor(connlib): spawn dedicated threads for UDP sockets#7590
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
4b579bb to
7176542
Compare
|
@jamilbk Initial measurements don't show a performance improvement from this unfortunately. I had hoped that it would further increase throughput. However, it lays the groundwork for spawning multiple threads for the UDP sockets (with It mightly slightly increase memory footprint (< 1MB) because we have new channels that can potentially be filled with buffers from the buffer pool. |
Based on some rudimentary CPU profiling I've been doing lately on the macOS client, it's possible this might introduce a healthy boost there. Hoping to find some undisturbed time to review this in depth today. Before this goes in, would it make sense to cut a new release for all affected components? We may want to isolate this change to a single patch release to allow for easy rollbacks if something comes up. |
We can! I am not sure if it is actually that risky? The main risk I see is roaming because we have more resources to clean up. But if that works, I think it should be pretty safe. |
Each thread we spawn also consumes 2MB on iOS minimum for the stack size, though that can be tweaked if we need to. |
jamilbk
left a comment
jamilbk
left a comment
There was a problem hiding this comment.
LGTM. Mostly following what's going on. Mostly refactoring due to the type changes and addition of async/await.
Can merge for testing after cutting next releases.
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
9e8d1c5 to
7928ca5
Compare
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
7928ca5 to
83c9d11
Compare
|
Can't measure a speed improvement with this, more the opposite. Throughput seems to drop by about 25% for some reason. I want to understand this better before merging this. |
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
353d4db to
22117f2
Compare
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
e40b0fe to
285ca84
Compare
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
b655b31 to
6b62654
Compare
thomaseizinger
force-pushed
the
refactor/udp-threads
branch
from
d06d018 to
069b881
Compare
|
I am merging this because it fixes the |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
The UDP socket threads added in #7590 are designed to never exit. UDP sockets are stateless and therefore any error condition on them should be isolated to sending / receiving a particular datagram. It is however possible that code panics which will shut down the threads irrecoverably. In this unlikely event, `connlib`'s event-loop would keep spinning and spam the log with "UDP socket stopped". There is no good way on how we can recover from such a situation automatically, so we just quit `connlib` in that case and shut everything down. To model this new error path, we refactor the `DisconnectError` to be internally backed by `anyhow`.
|
I believe this PR may be causing extreme packet loss on Apple for HTTP / TCP connections. Still gathering data. |
Generic Segmentation Offload (GSO) is a clever way of reducing the number of syscalls made when a you want to send a lot of packets with the same length to the same recipient. The way this works is that the packets are concatenated and passed to the kernel as a single packet together with the `segment_size` as an out-of-band argument. The component managing this batching in `connlib` is called `GsoQueue`. In #8772, we made the order in which these batches are sent to the kernel explicit by prioritising batches with smaller segments. What we overlooked with that strategy is that in a particular GSO batch, the last packet is actually allowed to be of a different length. For example, say the user is downloading an image of 4500Kb. With our MTU of 1280, we have a payload size of 1252. This results in three fully-filled packets and one packet of 744 bytes. With the change in #8772, the small packet of 744 bytes will be transferred first, followed by the "train" of fully filled packets. To fix this, we flip the order here and transfer batches or larger sizes first. The original problem we attempted to mitigate in #8772 no longer exists now that we merged #7590. We will simply suspend now if the UDP socket isn't ready contrary to dropping the next batch. By flipping the order here, we guarantee that batches with a larger size are sent before batches with a smaller size. This should also imply that the encapsulated IP packets of e.g. an image arrive in the correct order (with the smallest packet last as it is part of a smaller batch). What we don't guarantee with this is that there won't be any other IP packets sent "in the middle" of such a batch. This shouldn't be a problem though as we are simply interleaving packets of different TCP / UDP connections with each other which already happens on the regular Internet anyway.
With #7590, we've moved all UDP IO operations to a separate thread. As a result, some of the error handling of IO errors within the Client's and Gateway's event-loop no longer applied as those are now captured within the respective thread. To fix this, we extend the type-signature of the receive-channel to also allow for errors and use that to send back errors from sending AND receiving UDP datagrams.
2 participants
Correctly implementing asynchronous IO is notoriously hard. In order to not drop packets in the process, one has to ensure a given socket is ready to accept packets, buffer them if it is not case, suspend everything else until the socket is ready and then continue.
Until now, we did this because it was the only option to run the UDP sockets on the same thread as the actual packet processing. That in turn was motivated by wanting to pass around references of the received packets for processing. Rust's borrow-checker does not allow to pass references between threads which forced us to have the sockets on the same thread as the packet processing.
Like we already did in other places in
connlib, this can be solved through the use of buffer pools. Using a buffer pool, we can use heap allocations to store the received packets without having to make a new allocation every time we read new packets. Instead, we can have a dedicated thread that is connected toconnlib's packet processing thread via two channels (one for inbound and one for outbound packets). These channels are bounded, which ensures backpressure is maintained in case one of the two threads lags behind. These bounds also mean that we have at most N buffers from the buffer pool in-flight (where N is the capacity of the channel).Within those dedicated threads, we can then use
async/awaitnotation to suspend the entire task when a socket isn't ready for sending.Resolves: #8000