quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

Do not require &mut self in AsyncUdpSocket::poll_send #1519

Closed dignifiedquire closed 1 year ago

dignifiedquire commented 1 year ago

I am currently running into the issue that it requires a large amount of locking when implementing a custom AsyncUdpSocket, because poll_send at the moment requires &mut self.

So I looked into the reason that is (given that a regular UdpSocket::send does not require mutable access), and it turned out to only need write access to the last_send_error time. As the simplest option I put that behind a Mutex, which I think will be fine, but open to discussion of other variants, e.g. storing it in an atomic type or similar.

Ralith commented 1 year ago

Can you elaborate a bit on why implementing AsyncUdpSocket required locking? Unique access is something provided to the implementer by the caller, i.e. quinn.

dignifiedquire commented 1 year ago

Can you elaborate a bit on why implementing AsyncUdpSocket required locking?

The reason is that I have multiple different transports hiding behind it in my implementation. Depending on the hole punching/NAT situation it will either use a relay or actual UDP sockets, but those might change as well, depending on current connectivity.

So my solution is to use an RwLock to guard the inner state, such that reads and writes from quinn can grab a read lock for cheap, and only if an actual change happens, the write lock is acquired, applies the changes, and then release it.

Ralith commented 1 year ago

If you have &mut self you don't need any kind of lock, though; you can mutate state directly. If anything, it sounds like it'd be more useful if we added that to poll_recv as well.

dignifiedquire commented 1 year ago

&mut self is unfortunately not enough, as the underlying struct is wrapped in an Arc, as I need to be able to clone it around, e.g.

#[derive(Clone)]
struct MyConn {
  inner: Arc<RwLock<UdpSocket>>,
}

impl quinn::AsyncUdpSocket for MyConn {
  // .. 
}