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

First whack at UDP send/receive sockets for Mio #35

Merged
merged 1 commit into from
Oct 24, 2014
Merged

First whack at UDP send/receive sockets for Mio #35

merged 1 commit into from
Oct 24, 2014

Conversation

pvachon
Copy link
Contributor

@pvachon pvachon commented Oct 20, 2014

Add initial support to MIO for UDP sockets, including the ability to
join multicast groups and listen. Still need to add support for
recvfrom/sendto, but works just fine for connected UDP sockets.

Depends on nix-rust having pull request nix-rust/nix#12

@pvachon
Copy link
Contributor Author

pvachon commented Oct 21, 2014

Hopefully Travis will pass when pull request nix-rust/nix#12 is merged in. However, this pull request now includes support for recvfrom/sendto to maximize usefulness of UDP sockets.

@carllerche
Copy link
Member

I'm restarting the travis build. I will try to review the PR tonight!

@carllerche
Copy link
Member

I looked over the patch. Seems like a good start. I am going to bike shed it some but I am not super familiar w/ UDP so I may say some dumb things.

Also, it looks like the travis fail was due to a change in the Rust stdlib. I fixed master.



// Unconnected socket sender -- trait unique to sockets
pub trait IoUnconnectedSender {
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other uses for these two traits besides for UDP sockets? It seems like it will probably be better to keep these traits mio::net::udp unless there are other use cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question -- the rationale I had for splitting these out was actually related to UNIX domain sockets. When you have multiple potential clients to a single "listening" UNIX domain socket, you need to use recvfrom/sendto to make sure your message gets to the right destination. Thus, you could treat AF_UNIX sockets like AF_INET[6], SOCK_DGRAM sockets. This also helps for AF_RAW sockets and such, but I'd be surprised if anyone went that far. My thought was that there could be applications that are exclusively directed datagram oriented, so an AF_UNIX, and UDP socket could implement the same traits.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, but it's only sockets then? So maybe the trait can be UnboundSocket in net.rs? Also, does it make sense to split it up into 2 traits vs. a single UnboundSocket? As in, can there be a reader w/o a writer or the other way around?

If you can do this and rebase, I can merge into master (assuming all tests pass of course :)). Any further tweaks can happen on the main branch.

@carllerche
Copy link
Member

Overall it looks pretty good. My main thought would be to not bother w/ IoUnconnectedSender and IoUnconnectedReceiver and just implement them directly on UdpSocket (unless there is a good reason to have them around that I am not aware of).

Alternatively, have UdpSocket be a trait that implements send_to and recv_from, and have UnboundUdpSocket & BoundUdpSocket both implement it. BoundUdpSocket would also impl IoReader and IoWriter and UnboundUdpSocket would impl bind() -> BoundUdpSocket. What do you think of this? Good idea or too much API overhead for not much win?

Add support to mio for listening and managing UDP sockets, including
infrastructure for supporting unconnected sockets (i.e. using
recvfrom/sendto) and appropriate interfaces to be exposed for
connectionless sockets.

Additionally, adds support for multicast listener/transmitter sockets.
@pvachon
Copy link
Contributor Author

pvachon commented Oct 23, 2014

Cool, I just made the change to the UnconnectedSocket trait (and moved it to net.rs) -- I think that's a much cleaner approach to the API, and it's unlikely you'll have a case where you'll only want sendto OR recvfrom. Let me know if you have any suggestions or feedback!

@carllerche
Copy link
Member

Looks good to me. I'll merge it. Do you mind if I rebase into 1 commit? I can keep the author info 👍

@pvachon
Copy link
Contributor Author

pvachon commented Oct 23, 2014

Damnit, I thought I did rebase it into one commit -- but yeah, definitely OK by me.

@carllerche carllerche merged commit b9b308f into tokio-rs:master Oct 24, 2014
@carllerche carllerche mentioned this pull request Oct 24, 2014
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