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

Manually poll an UdpSocket is not working, but used to – regression? #955

Closed
Frando opened this issue Feb 12, 2021 · 5 comments
Closed

Comments

@Frando
Copy link

Frando commented Feb 12, 2021

Hi,
I need to manually poll an UdpSocket to drive it from a poll_next method.
I've read #636, where @stjepang suggests to just recreate the future on each poll. I've set up an example here:

https://github.com/Frando/async-std-udp-test/blob/main/src/main.rs

When pinning async_std to =1.6.2 this works as it should. When changing the async_std version to 1.9, the example fails: The reading side is not woken after a first read_from returns Poll::Pending.

Is there something I'm missing, or is this a regression?

I'll try to track down from which version it fails (the 1.6.2 comes from another crate that depended on this version, from where I started debugging why things don't work anymore).

@Frando
Copy link
Author

Frando commented Feb 12, 2021

OK, I pinned down where the regression occurs: It works fine with 1.6.3 but not with 1.6.4.

@ghost
Copy link

ghost commented Feb 12, 2021

This "worked" previously because we used a hack, which was later on removed. Imagine there are 1000 tasks concurrently polling read_from on the same UdpSocket. We'd have to store all those wakers somewhere and then wake them all up at once when the UdpSocket becomes readable. That's an extreme and contrived example, but you see how inefficient it could be in theory. The hack we used before was to store a bounded number of wakers (a few dozen of them) and immediately wake all wakers when the limit is reached. But now, when a read_from future is dropped, it removes its registered waker because we assume that the read_from operation was canceled.

What you should do instead is store the future inside UdpStream struct. Take a look at this for inspiration: https://docs.rs/async-net/1.5.0/src/async_net/tcp.rs.html#257-259

@Frando
Copy link
Author

Frando commented Feb 13, 2021

Thank you for the explanation! Storing the future was actually what I tried first, but ran into lifetime issues and then found that comment of yours. I'll try again storing the future then.

@Frando
Copy link
Author

Frando commented Feb 13, 2021

OK, I found a way to circumvent the lifetime issues. I don't think its super elegant, but it works: I'm moving both the socket and the buffer into the future, getting it back when the future is ready.
Frando/async-std-udp-test@4b0dc43
It works and will do for now I hope. Still have to see if I can make that compatible with sending over the socket at the same time (or making it appear as such from the outside). I'd prefer a way where I can store just the future without moving the socket in. I think that's doable as well, but not sure if it's possible without unsafe, and so far I never dealt with unsafe future and pinning things. If there's an example somewhere I'd be interested.

Frando added a commit to datrs/hyperswarm-dht that referenced this issue Feb 15, 2021
- The poll_next method of UdpFramed needs to store the socket.recv_from
  future, the old behavior of recreating the future on each poll is no
  longer supported.
  See [this issue](async-rs/async-std#955)
  for details.
- I couldn't get the future to work with BytesMut because all references
  or raw pointers seem to be not Send which is needed for it to work
  across the await. Thus I changed to a vec, which should be fine too,
  but not sure.
@Frando Frando closed this as completed Feb 25, 2021
@Frando
Copy link
Author

Frando commented Feb 25, 2021

I found a more elegant way to store and poll the future than in the example linked above, see https://github.com/Frando/async-osc/blob/72ec4cc7aac65891a241daab9d180b2ace636c70/src/udp.rs if interested.

Closing this, thanks for the help 🙂

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

No branches or pull requests

1 participant