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

UdpSocket split support #1226

Merged
merged 1 commit into from
Jul 8, 2019
Merged

UdpSocket split support #1226

merged 1 commit into from
Jul 8, 2019

Conversation

blckngm
Copy link
Contributor

@blckngm blckngm commented Jun 29, 2019

The UDP counterpart of #1217.

A specialized split is implemented for UdpSocket, making it possible to receive and send on the socket concurrently, even from two different tasks.

There are no AsyncRead or AsyncWrite like traits for UdpSocket. So the halves have receive or send methods same with the socket. The methods return the same future types with their UdpSocket equivalents.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

This seems reasonable though I'd like to have @jonhoo or @carllerche look at it.

tokio-udp/src/split.rs Show resolved Hide resolved
@carllerche carllerche mentioned this pull request Jul 8, 2019
2 tasks
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks! I tracked polish questions on #1250.

@carllerche carllerche merged commit 88e775d into tokio-rs:master Jul 8, 2019
@blckngm blckngm deleted the udp-split branch July 9, 2019 06:01
@ghost
Copy link

ghost commented Oct 3, 2019

Why the API of UdpSocket::split is inconsistent with TcpStream::split? The former takes self while the latter borrows &mut self.

UdpSocket::split:

fn split(self) -> (UdpSocketRecvHalf, UdpSocketSendHalf)

TcpStream::split:

fn split(&mut self) -> (ReadHalf, WriteHalf)

What's more, UdpSocketSendHalf::send_to accepts only SocketAddr while UdpSocket::send_to accepts impl ToSocketAddrs.
UdpSocketSendHalf::send_to:

fn send_to(
    &'_ mut self,
    buf: &'_ [u8],
    target: &'_ SocketAddr
) -> impl Future<Output = Result<usize, Error>>

UdpSocket::send_to:

fn send_to<A>(
    &'_ mut self,
    buf: &'_ [u8],
    target: A
) -> impl Future<Output = Result<usize, Error>> where
    A: ToSocketAddrs

I have written a patch to make these API feel consistent, but I'm not sure whether it is considered a bug.

@blckngm
Copy link
Contributor Author

blckngm commented Oct 3, 2019

Hi,

Why the API of UdpSocket::split is inconsistent with TcpStream::split? The former takes self while the latter borrows &mut self.

UdpSocket::split:

fn split(self) -> (UdpSocketRecvHalf, UdpSocketSendHalf)

TcpStream::split:

fn split(&mut self) -> (ReadHalf, WriteHalf)

Originally TcpStream::split also takes ownership (#1217). The split_mut method that takes &mut self was added in #1289. Later split was replaced by split_mut in #1521.

But I think it would problematic if UdpSocket loses split(self). The borrowed halves produced by split(&mut self) would be very difficult to use across multiple tasks.
TcpStream and UnixStream can at least use io::split, but there is no equivalent for UDP.

If we change UdpSocket::split to take &mut self for API consistency, I recommend that split(self) is kept and renamed, to e.g. split_owned.

What's more, UdpSocketSendHalf::send_to accepts only SocketAddr while UdpSocket::send_to accepts impl ToSocketAddrs.
UdpSocketSendHalf::send_to:

fn send_to(
    &'_ mut self,
    buf: &'_ [u8],
    target: &'_ SocketAddr
) -> impl Future<Output = Result<usize, Error>>

UdpSocket::send_to:

fn send_to<A>(
    &'_ mut self,
    buf: &'_ [u8],
    target: A
) -> impl Future<Output = Result<usize, Error>> where
    A: ToSocketAddrs

I have written a patch to make these API feel consistent, but I'm not sure whether it is considered a bug.

The ToSocketAddrs mechanism was added later. I agree that it should be used in the send half now.

@ghost ghost mentioned this pull request Oct 4, 2019
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