Skip to content

Add UdpSocket::{poll_recv_from, poll_send_to} #636

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

Closed
wants to merge 1 commit into from

Conversation

Demi-Marie
Copy link

This is needed for QUIC implementations based on async-std. It could be
done with UdpSocket::{recv_from, send_to}, but this requires boxing,
reference counting, and unsafe code.

@yoshuawuyts yoshuawuyts requested a review from a user January 10, 2020 15:36
@yoshuawuyts
Copy link
Contributor

@demimarie-parity hi, thanks for opening this PR! -- This is a bit of a design departure from where we were at. The team has been away over the holidays, but we're meeting up again next week to catch up and discuss future directions. I'll make sure we discuss this PR then!

Also if Parity is experimenting with async-std, I'm sure I speak for the team we would love to hear more about the directions you're taking to ensure we can support you. We'd love to see you succeed!

@ghost
Copy link

ghost commented Jan 13, 2020

It could be
done with UdpSocket::{recv_from, send_to}, but this requires boxing,
reference counting, and unsafe code.

I believe poll_recv_from() can be implemented using recv_from() without any problems:

pub fn poll_recv_from(
    &self,
    cx: &mut Context<'_>,
    buf: &mut [u8],
) -> Poll<io::Result<(usize, SocketAddr)>> {
    Pin::new(&mut self.recv_from(buf)).poll(cx)
}

So you could simply do Pin::new(&mut self.recv_from(buf)).poll(cx) instead of using poll_recv_from(). The reason why this works is that the recv_from() future has no state and does nothing on drop -- it only forwards the call to watcher.poll_read_with().

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This is needed for QUIC implementations based on async-std.  It could be
done with `UdpSocket::{recv_from, send_to}`, but this requires boxing,
reference counting, and `unsafe` code.
@Demi-Marie
Copy link
Author

@stjepang Two caveats:

  • First, this adds unnecessary overhead. The compiler may be able to optimize it away, but I would rather not count on that.
  • Second, this is contrary to the normal behavior of futures, so it would need to be documented.

@yoshuawuyts
Copy link
Contributor

First, this adds unnecessary overhead. The compiler may be able to optimize it away, but I would rather not count on that.

Pin is a zero-sized type, and LLVM is incredibly good at inlining when compiling Rust with --release. Even if this failed to optimize (which seems unlikely) the performance impact would be limited. Overall this doesn't seem like a convincing enough argument to warrant an API addition.


Second, this is contrary to the normal behavior of futures, so it would need to be documented.

Do you have any suggestions to what this could look like? The pinning chapter in the async book goes into this a bit. And so do the std::pin docs. But not too well.

Maybe there is something there we could do to improve this? Or do you think an async-std specific addition would be more helpful?

@Demi-Marie
Copy link
Author

The biggest issue is that async functions don’t implement Unpin, so I need to either box the future (which is actual overhead) or use the unsafe Pin::new_unchecked function. Such use is correct, but users should not be forced to write unsafe code when it is not necessary.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Jan 27, 2020

@demimarie-parity pin-project and pin-project-lite are the recommended ways of replacing the unsafe code with safe compile-time invariant checks. The stdlib doesn't include either, but the Pin API was very much designed with these abstractions in mind.

Either way, I think we can have a resolution for this issue. Going to go ahead and close this PR and #634!

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.

None yet

2 participants