Skip to content

Add send(), recv() and connect() to UDPSocket.#567

Merged
alexcrichton merged 5 commits into
tokio-rs:masterfrom
vvanders:master
Mar 17, 2017
Merged

Add send(), recv() and connect() to UDPSocket.#567
alexcrichton merged 5 commits into
tokio-rs:masterfrom
vvanders:master

Conversation

@vvanders

Copy link
Copy Markdown
Contributor

Hey there!

I was looking to bring parity between net::UdpSocket and udp::UdpSocket by adding connect(), send() and recv() on UDP sockets.

I've tested my changes on Win10 and Ubuntu 16.04(in a VM) with all unit test passing. If there's any adjustments you'd like to make to the PR I'd be happy to accommodate them. I figured this was a small addition so it didn't need a separate issue, happen to open one if that's not the case.

Thanks,
Val

@alexcrichton

Copy link
Copy Markdown
Contributor

Thanks for the PR! I wonder, should the Windows version only use recv after the socket's been connected? It looks like it may still be using recv_from unconditionally? (may also be good to add a test for this if that's true)

@vvanders

Copy link
Copy Markdown
Contributor Author

That's actually by design, connecting a UDP socket doesn't do anything more than set an explicit destination address for send() and filter packets that don't match that address on the port. Recvfrom is valid for both connected and unconnected sockets(https://msdn.microsoft.com/en-us/library/windows/desktop/ms740120(v=vs.85).aspx and https://linux.die.net/man/2/recvfrom).

Since windows speculatively reads from the socket in order to do io completion and we don't know if the client will call recv() or recv_from() so we can use recvfrom() to get the additional address information and the discard it if it's a recv() call. I've got tests in miow and in mio that cover this case.

Let me know if that makes sense or if you want a bit more detailed breakdown, I can also add a few more comments around the windows section if that would help as well.

@alexcrichton

Copy link
Copy Markdown
Contributor

Oh! Right! duh :)

Carry on!

Comment thread src/sys/windows/udp.rs
}
}

pub fn recv(&self, mut buf: &mut [u8])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this call the impl above and just throw away the address if it comes out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like that a little bit better. We do a bit more work but that's already the case in the windows impl but it should be much cleaner and easier to reason about.

@alexcrichton

Copy link
Copy Markdown
Contributor

Could you add a test that a packet from a different address is ignored?

@vvanders

Copy link
Copy Markdown
Contributor Author

Yup, will add that test case, might take me a little bit since I'll have to rework some of the tests to check for not receiving anything.

@alexcrichton

Copy link
Copy Markdown
Contributor

Looks great! I've pushed to the dev branch to run tests again

@alexcrichton alexcrichton merged commit 523b2aa into tokio-rs:master Mar 17, 2017
@vvanders

Copy link
Copy Markdown
Contributor Author

Awesome, thanks for accepting the PR! Happy to contribute where I can.

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.

2 participants