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

add UnboundUdpSocket to std::net #97672

Closed
wants to merge 1 commit into from

Conversation

keepsimple1
Copy link

@keepsimple1 keepsimple1 commented Jun 3, 2022

(Thanks to the original discussions in Rust internal forum, I finally found some courage to open this PR, thinking it is not something needs a RFC based on what I learned. Of course I might be totally wrong. Any comments are greatly appreciated. )

Motivation

Currently std::net does not support socket configurations before binding to an address. For example, set SO_REUSEADDR is common for UDP sockets and it is not possible using std::net.

Proposal

Add UnboundUdpSocket in std::net to support UDP socket configurations before binding to an address. Once bound, the socket will become a regular UdpSocket.

Similarly UnboundTcpSocket is to support TCP socket configurations before connecting or binding.

Implementation

The implementation is straightforward and kept to minimum at this moment.

Note1: for UnboundUdpSocket, only configurations of SO_REUSEADDR is implemented in its public API. It is because what I needed, also because adding more methods is not the key point of this patch.

Note2: for UnboundTcpSocket, because the current codebase already sets SO_REUSEADDR for TcpListener, this diff only prepares for UnboundTcpSocket by implementing its net_imp part. I did not have a specific need to add for TCP at this moment, hence did not implement its public API.

Note3: UnboundUdpSocket is also useful for Windows platforms by supporting SO_EXCLUSIVEADDRUSE.

Also, the issue number 99999 is a placeholder.

Prior art

The most popular crate for socket programming is probably socket2. However, I believe std::net is worth to be enhanced to support socket operations before binding.

Thanks

Again, thanks for all comments in the Rust internals discussion I linked at the top. They are very helpful and encouraging.

EDIT:

  • I also added a new SocketAddrFamily so that we can create a Socket without a full SocketAddr.

EDIT:

  • Issue number changed to none as the number 99999 was taken recently.

EDIT:

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 3, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2022
@keepsimple1
Copy link
Author

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 3, 2022
@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 3, 2022
@rust-log-analyzer

This comment has been minimized.

library/std/src/net/udp.rs Outdated Show resolved Hide resolved
library/std/src/net/addr.rs Outdated Show resolved Hide resolved
library/std/src/sys_common/net.rs Outdated Show resolved Hide resolved
library/std/src/sys_common/net.rs Outdated Show resolved Hide resolved
library/std/src/sys_common/net.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jnordwick
Copy link

jnordwick commented Jun 5, 2022

Once bound, the socket will become a regular UdpSocket

Some config doesn't inherit through an accept call (eg, ioctl flags), so you might still need a way to do that, and some config is required to be done before accept (eg sdn/rcvbuf). There's also multicast join/leave stuff too that I'm not sure how you fit in. I had to write all this myself a few months ago already, it was such a pain to incorporated into other libs that I wound up writing my own networking layer and protocol stack.

@keepsimple1
Copy link
Author

Some config doesn't inherit through an accept call (eg, ioctl flags), so you might still need a way to do that,

At the public API level, we won't expose ioctl (nor setsockopt for that matter). So as long as we can provide the functions needed, we will be okay. For example, today std::net supports set_nonblocking via ioctl. I don't get what you mean by doesn't inherit through an accept call. UDP does not have accept call, but besides that, could you mind give an example of such case to help me understand?

and some config is required to be done before accept (eg sdn/rcvbuf).

This is where this PR comes in. It provides a basis to support such configs. In current diff, we only implemented SO_REUSEADDR, but any other config to be done before bind can/will be supported. ( By accept, I assume you mean bind. )

There's also multicast join/leave stuff too that I'm not sure how you fit in.

It is already supported by UdpSocket today at here.

@jnordwick
Copy link

I hope this is planned so it will work with both TCP and UDP, even if UDP is your current need. It would suck to have to have multiple ways to construct a socket.

I was referring to socket opts for TCP, but UDP has a couple too. I read the internals discussion, and the unbound state and builder idea was to allow for configuration calls before the socket is actually created, from my understanding.

But there isn't just one point for config, there are two for TCP in the case of an accepting socket. The C sematics are ugly because it was done piecemeal and we definitely do not want to keep the C calls structure since it would create portability issues. Eg, to create a non-blocking server tcp socket, you cannot set non-blocking on the socket itself before accept. That's what I mean by not inherited through the accept call. (There is a small set of these that have to wait until after accept).

The nice thing would be to set it on the some builder or prototype socket and have it handled whenever. This way each networking library doesn't need to hand the socket back to you for post-accept config before starting to use it internally.

It is often considered bad and non-portable to rely on accept applying any options to the spawned socket since that varies by system. Best to just set them all again. So just to be portable, those options need to stick around somehow.

Another one is SO_ZEROCOPY and I don't know how that is handled..

And of course the net socket family should work with getaddrinfo and friends. There are a couple dns crates, and it would be good to make sure it doesn't interact poorly with any of them.

I think the current attempt might lead to difference in socket config by system if used for accepting sockets.

@jnordwick
Copy link

It is already supported by UdpSocket

The problem with that is you have to build a way to change these options on the socket after whatever runtime or system has taken the socket over. Instead of relying on each system to write in a door to do join/leave calls.

It might be wroth thinking of if we want to allow a those calls from another part of the system. I thinkg it isn't a bad idea, much easier than trying to grab the scoket from the runtime when we need to change group or source memberships.

@keepsimple1
Copy link
Author

@jnordwick

I hope this is planned so it will work with both TCP and UDP, even if UDP is your current need. It would suck to have to have multiple ways to construct a socket.

TCP support follows the same way. In fact, the underlying support is already in this PR (look for UnboundTcpSocket in sys_common/net.rs.

I was referring to socket opts for TCP, but UDP has a couple too. I read the internals discussion, and the unbound state and builder idea was to allow for configuration calls before the socket is actually created, from my understanding.

Not before actually created, rather it is before bind.

But there isn't just one point for config, there are two for TCP in the case of an accepting socket. The C sematics are ugly because it was done piecemeal and we definitely do not want to keep the C calls structure since it would create portability issues. Eg, to create a non-blocking server tcp socket, you cannot set non-blocking on the socket itself before accept. That's what I mean by not inherited through the accept call. (There is a small set of these that have to wait until after accept).

This is fine. For TCP, std::net supports configs for both points, and with UnboundTcpSocket, will support config before listen too.

The bottom line is: this part ofstd::net is really just a nice wrapper API to the underlying system socket. For any particular socket feature, whether it is inherited through the accept call, it is same for std::net as for the C socket calls.

Of course, the current std::net does not support any socket configs before listen (or bind) but this PR aims to fix that.

The nice thing would be to set it on the some builder or prototype socket and have it handled whenever. This way each networking library doesn't need to hand the socket back to you for post-accept config before starting to use it internally.

It is often considered bad and non-portable to rely on accept applying any options to the spawned socket since that varies by system. Best to just set them all again. So just to be portable, those options need to stick around somehow.

In my opinion, this particular issue is not in the scope of the std::net. Out of curiosity, do you have any pointers to a long-form discussion about why it's "bad and non-portable"? or an example of doing the portable way?

Another one is SO_ZEROCOPY and I don't know how that is handled..

I don't think it's supported in std::net today. But again, it is a matter of adding a wrapper API. It really depends on the underlying system.

And of course the net socket family should work with getaddrinfo and friends. There are a couple dns crates, and it would be good to make sure it doesn't interact poorly with any of them.

getaddrinfo does not interact with the socket calls in this PR. If you think otherwise, please elaborate. An example use case will be good.

@keepsimple1
Copy link
Author

It is already supported by UdpSocket

The problem with that is you have to build a way to change these options on the socket after whatever runtime or system has taken the socket over. Instead of relying on each system to write in a door to do join/leave calls.

I'm not sure what you mean by whatever runtime or system has taken the socket over. UdpSocket in this case is the owner of the underlying socket. Any join/leave should go through the UdpSocket.

@jnordwick
Copy link

jnordwick commented Jun 10, 2022

I'm not sure what you mean by whatever runtime or system has taken the socket over

I was hoping this would solve my issue with not being able to use things like tokio or other network libs in the rust ecosystem effectively. I need to do things like setting the snd and recv buffer sizes, setting non/blocking, zerocopy, and a few others. At present this isn't possible without ugly hacks in any package and make rust networking code a fairly bad experience.

Very rarely do you get to keep the socket you are creating. Often you wrap it in something else to hand back to whatever network libs, system, or async runtime you are using.

So under this system the socket config is scattered and sometimes still not possible without the same ugly hacks (eg, if you are using tokio there is still no way to config a server side tcp socket).

@keepsimple1
Copy link
Author

keepsimple1 commented Jun 10, 2022

I was hoping this would solve my issue with not being able to use things like tokio or other network libs in the rust ecosystem effectively. I need to do things like setting the snd and recv buffer sizes, setting non/blocking, zerocopy, and a few others. At present this isn't possible without ugly hacks in any package and make rust networking code a fairly bad experience.

If I understand, you won't be able to use types from std::net directly and had to drop down to raw socket to do things, or hand over the raw socket to other libs.

Although the current std::net TCP is missing features you wanted, have you tried the crate async-io ? I've used its Async adaptor with std::net::UdpSocket, and it has been working for me pretty well.

Also, I'm not familiar with tokio. I used smol as the runtime, which consists of async-io.

With this PR, I hope std::net can move to the direction of providing full set of socket configurations, so that it motivates the users to work with std::net types directly instead of dealing with raw sockets.

@jnordwick
Copy link

With this PR, I hope std::net can move to the direction of providing full set of socket configurations, so that it motivates the users to work with std::net types directly instead of dealing with raw sockets.

Under the way you are proposing, I still don't think you get there. You might have cover another usecase, but there are still a couple others where yet another parital API would need to be done to cover, sot there would essentially be three different ways, all necessary, to cover some some socket use cases. Even then, they are likely to not be able to set the same things in the same ways on different OSes - this is the warning to not assume your setsockopt calls will be inherited by the accepting socket since that is implementation dependent and this might only work on your unix variant (I'm unsure as to the extent of that difference though).

If I understand, you won't be able to use types from std::net directly and had to drop down to raw socket to do things, or hand over the raw socket to other libs.

Not quite what I was trying to get across. When most people use rust networking it will be with something else, even something simple like the tungsten websocket API takes control of the sockets. Tokio (I've use it and std-async - which uses smol I believe) has a was to hand it a socket and it will take ownership, but your config chances end there. The example I was using was a server side tcp socket that a generated by accept. There would still not be a way to control its config completely on in a OS-independant way. You can, but it wasn't easy for me to do (but I'm not the greatest Rust dev).

In order to do full socket configuration in a portable way, you need to also be able to do config at a couple other points in the lifecycle of the socket (eg, after accept and potentially before close). The issue is that you usually don't maintain ownership of the socket and don't have a reference to it, so it becomes impossible to manipulate when needed. Or you just leave it up to every package to provide the calls or hooks for this. I'm guessing that wouldn't work out too well.

If you want to main the thinnest layers over the C API, this probably isn't possible. You start to have conflicting goals/requirements. Last time I dealt with this I had to make a couple small compromises (I created a handle to fd that could be used to manipulate the fd, but I had needs that were a little more sophisticated than most).

Rust's networking is lacks cohesion so it can be difficult to figure out how to fix it piecemeal without just wanting to tear it down, but I definitely applaud you're attempt. I don't think this is far off in the wrong direction given your goals, but there are a few things are are going to be left out. It will still be an improvement. Good luck.

I would hate to see someone do all this work only to have to retool it as soon as the next person needed something done.

@keepsimple1
Copy link
Author

@m-ou-se since you are one of the assignees, may I ask if you have any thoughts or comments about this PR? Thanks.

@bors
Copy link
Contributor

bors commented Jun 15, 2022

☔ The latest upstream changes (presumably #95897) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@keepsimple1
Copy link
Author

@m-ou-se could you please take a look at this PR? Thanks!

@bors
Copy link
Contributor

bors commented Jul 3, 2022

☔ The latest upstream changes (presumably #97437) made this pull request unmergeable. Please resolve the merge conflicts.

@jkerdreux-imt
Copy link

I tested @keepsimple1 patch with great success. I even added reuseport. But I'm wondering :

  • Why the std::net still doesn't provide this kind of API for long time ? I mean, UDPSocket without some kind of setsockopt is useless. That's why most people use socket2, but it just add another layer which is not compliant w/ async_socket crate for example ?

  • Can we get ride of UnboundUDPSocket and simply implement the UDPSocket::new the way it should be ? I mean unbound by default. I found this kind of request for years, and this is the way all other programming language implements this stuff too. I understand @keepsimple1 added Unbound*Socket to have a chance to get his code upstream, but we knows this not the right way to do.

Thanks

@rustbot
Copy link
Collaborator

rustbot commented Jul 7, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@keepsimple1
Copy link
Author

@jkerdreux-imt Thanks for testing the patch! I'm really glad to hear that it works for you. Regarding your 2nd question:

Can we get ride of UnboundUDPSocket and simply implement the UDPSocket::new the way it should be ?

I think one probably would adopt UDPScoket::new if they started from scratch. But the current semantics of std::net::UDPSocket is that it is always bound. This is not explicitly stated in its doc, but it's true in practice. For example, it is expected that you can always call recv_from on a UDPSocket.

If we were to implement UDPSocket::new, then we have to add & manage extra state inside the struct for binding. In my opinion, this will lead to more complex implementation and API, and might break existing code.

And, another way of looking at adding UnboundUDPSocket is that it's a case of theTypestate pattern (even though I didn't think of the pattern when working on the patch).

@keepsimple1
Copy link
Author

Just to clarify, I didn't know why rustbot commented again like doing with a new PR. I just resolved a new conflict with master branch using GitHub's online tool. I suspect that rustbot might not be familiar with that workflow and doesn't know how to handle it.

@keepsimple1
Copy link
Author

@m-ou-se I just found out the new ACP process, should I create one ACP for this PR?

@jnordwick
Copy link

jnordwick commented Jul 9, 2022

@keepsimple1 Is AF_UNSPEC supported? The modern way of open a net connection in C makes it so you dont have to touch the address structures or speficy IP version. The call structure is now:

struct addr_info hints, *list;
... code fills in hints  using hints.ai_family = AF_UNSPEC ...
getaddrinfo(0, "http", &hints, &list);
itn s = socket(0, list[0].ai_family, list[0].ai_socktype, list[0].ai_protocol); 
// normally you run through the list
// until you get a connection using list[0].ai_next
bind(s, list[0].ai_arrd, lis[0].ai_addrlen);

In that call sequece you don't ahve to know which IP version dns is going to reutrn and let the os handle that do ryou.

@keepsimple1
Copy link
Author

@jnordwick

@keepsimple1 Is AF_UNSPEC supported?

AF_UNSPEC is used by getaddrinfo, which IMO is orthogonal to the socket API. Hence it is not in the scope of this PR.

@jnordwick
Copy link

which IMO is orthogonal to the socket API

getaddrinfo is the preferred way to create arguments for calls to socket and connect now to allow for IP version agnostic sockets.

@keepsimple1
Copy link
Author

force-pushed to get rid of the merge commit. (Lesson to myself: don't use GitHub online tools to resolve the conflicts).

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 31, 2022

☔ The latest upstream changes (presumably #78802) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Sep 13, 2022

☔ The latest upstream changes (presumably #100640) made this pull request unmergeable. Please resolve the merge conflicts.

@keepsimple1 keepsimple1 force-pushed the unbound-socket branch 2 times, most recently from b86fd03 to 76d295f Compare September 28, 2022 20:22
@bors
Copy link
Contributor

bors commented Oct 14, 2022

☔ The latest upstream changes (presumably #103048) made this pull request unmergeable. Please resolve the merge conflicts.

… before binding to an address. Once bound, the socket becomes a regular UdpSocket.
@pitaj pitaj added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2023
@the8472
Copy link
Member

the8472 commented May 18, 2023

Closing since the ACP was rejected.

@the8472 the8472 closed this May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.