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

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

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Mar 20, 2023

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.

@dignifiedquire dignifiedquire changed the title refactor: do not require &mut self in AsyncUdpSocket::poll_send Do not require &mut self in AsyncUdpSocket::poll_send Mar 20, 2023
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me!

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should generally avoid locking in I/O, but this should be well off the hotpath so I think it's fine. Easy to swap in an atomic microsecond counter or something later if we need to.

@Ralith
Copy link
Collaborator

Ralith commented Mar 20, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

Ralith commented Mar 24, 2023

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.

@djc djc merged commit 75524fc into quinn-rs:main Mar 24, 2023
@dignifiedquire
Copy link
Contributor Author

&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 {
  // .. 
}

@djc djc mentioned this pull request May 8, 2023
3 tasks
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.

3 participants