Amend RFC 517: Add material on std::net #807

alexcrichton · 2015-02-03T20:41:03Z

The IO reform RFC is being split into several semi-independent pieces, posted as PRs like this one.

This RFC amendment adds the networking section.

codyps · 2015-02-03T20:52:50Z

text/0517-io-os-reform.md

+* The static distinction between `TcpAcceptor` and `TcpListener` has been
+  removed (more on this in the [socket][Sockets] section).
+* The `clone` functionality has been removed in favor of `duplicate` (same
+  caveats as `TcpStream`).


What's the reasoning for not using clone here?

The Clone trait primarily indicates that the clone operation itself is infallible whereas the system call dup can fail (file descriptor exhaustion). I suspect that WSADuplicateSocket may also have some similar error conditions.

kjpgit · 2015-02-03T22:18:46Z

I had to look in the source code to see that SO_REUSEADDR was being set. It's useful, but should be documented and have an option. Also, SO_REUSEPORT (a newish option for linux/bsd) would be really, really nice to support as an option http://lwn.net/Articles/542629/

I have a threaded server that does concurrent accepts, on the same 'cloned' socket, and I was shocked to run strace and see that TcpListener was 1) creating a pipe 2) calling select() even though no timeout was set (something with close_accept). These were creating a thundering herd problem; select returns 'connection is available' to ALL threads, even though only one will then call accept successfully - the rest will get EAGAIN. If the threads were just calling accept() directly, no thundering herd should happen on new linuxes.

So I think this is doing in the right direction, if TcpListener can copy itself using dup(), and accept() with no timeout maps directly to an accept() syscall, without select() messing things up. That should work for 'basic cross platform'. If SO_REUSEPORT was supported, threads (or processes) could just all create their listeners from scratch and not dup anything. I'm trying to use 100% safe code, but I realize stuff like REUSEPORT isn't 100% portable, and you need it to be set before the bind() happens. So us app developers are in a "bind", pun intended...

Also remove some `&mut self` methods as only `&self` is necessary.

alexcrichton · 2015-02-03T23:10:29Z

@kjpgit

Also, SO_REUSEPORT (a newish option for linux/bsd) would be really, really nice to support as an option http://lwn.net/Articles/542629/

Thanks for the article link! It was a nice read :)

I'd definitely love to support this level of configuration, it's certainly been requested multiple times! I think that something like SO_REUSEPORT is also perfect for an os::unix addition (or even os::linux depending on availability) as an extension of a socket structure.

I think for now we may want to avoid these sorts of options or various configuration for creating new sockets. The provided constructors serve as supplying some reasonable defaults, and various other options can be supported through a separate constructor method.

I would personally prefer to defer the precise method of what a "highly configurable constructor" would look like due to the need to land these basic implementations somewhat quickly in-tree. Some thoughts I have on this topic, however, are:

We could provide a SocketOptions builder-style structure similar to OpenOptions for File which would instruct how to open a socket and what operations to perform. A downside of this, however, is that an error is tough to associate with which option caused the failure.
We could provide a Socket which can then be converted to one of the above types. A Socket would then expose all the various methods for configuration that are supported. A downside of this, however, is that it looks like it'll either not have a huge number of static guarantees or it's got an explosion of types to track.

Either way I think we definitely have some options for pursuing configuration of opening of each network socket type, but for now we shouldn't be closing any doors by taking the conservative option of only providing one default constructor. This is similar to the story for File::open vs File::open_opts for example.

kjpgit · 2015-02-03T23:19:50Z

text/0517-io-os-reform.md

+  a later date in a more robust fashion with `select`.
+* The `accept` function no longer takes `&mut self` and returns `SocketAddr`.
+  The change in mutability is done to express that multiple `accept` calls can
+  happen concurrently.


+1 for the concurrent accept. Seems pretty standard, and would work fine for my toy use case at least. EDIT: as long as there's no hidden select() giving you a gun-at-foot thundering herd problem...

nagisa · 2015-02-04T08:24:59Z

It is unclear how socket library initialisation is done on platforms where it is necessary. I.e. on Windows you must call WSAStartup before using sockets and WSACleanup after you finish with them. This RFC does not tell how this is (or is supposed to be) implemented. Are we staying with our current implementation which won’t even free resources after itself?

EDIT: this is API RFC, not an implementation one… Ignore my question

alexcrichton · 2015-02-04T17:05:19Z

@nagisa that could in theory affect the API actually if we decide to expose explicit setup/teardown functions, although I'm not sure that we would want to do so. I do think it is a bug, however, that we are not calling WSACleanup.

l0kod · 2015-02-05T22:58:54Z

text/0517-io-os-reform.md

+    ...
+}
+#[cfg(unix)]    impl AsRawFd for TcpListener { ... }
+#[cfg(windows)] impl AsRawSocket for TcpListener { ... }


It would be great to be able to create TcpStream, TcpListener, and UdpSocket from an AsRawFd or AsRawSocket as well (e.g. file descriptor passing or cross I/O library interface).

I agree that this would be quite useful! I think we may want to develop our story around these raw representations a little more before exploring this area, however. The AsRawFoo traits are pretty new and I'd want to make sure that they're sufficient before we start adding conversions in the other direction.

There is, however, other interest in this, so we may be able to at least sketch out something #[unstable] for now.

Cool! Could you add this as a new amendment of the RFC #517 to follow the discussion please?
Some ideas:

RFC: io and os reform: initial skeleton #517 (comment)

libs: add platform-specific APIs to extract file descriptors, SOCKETs, HANDLEs, etc from std::io rust#19169

MIO IoHandle and IoDesc

l0kod · 2015-02-05T23:03:12Z

cc rust-lang/rust#14194 (abstract UNIX domain socket)

nagisa · 2015-02-06T06:40:13Z

@l0kod this at best would be unsafe as you could create several objects from the same file descriptor and dropping (= closing) sockets multiple times is invalid.

l0kod · 2015-02-06T07:59:02Z

this at best would be unsafe as you could create several objects from the same file descriptor and dropping (= closing) sockets multiple times is invalid.

Not necessarily if you take ownership. Something like this: fn from_fd(fd: AsRawFd) -> IoResult<TcpStream>.
We could use an into_inner()-like to only keep the inner value without closing it (e.g. file descriptor) and drop the original wrapper.

alexcrichton · 2015-02-06T18:37:52Z

I have an in-progress implementation PR as well now: rust-lang/rust#22015

The PR specifically fleshes out many details of the IpAddr and SocketAddr types (many thanks to @ktossell in #20785!) and is otherwise largely faithful to the current revision of this RFC.

glennpratt · 2015-02-10T06:00:15Z

text/0517-io-os-reform.md

+
+Some important points of note are:
+
+* The `send` and `recv` function take `&self` instead of `&mut self` to indicate


send_to and recv_from?

frewsxcv · 2015-02-15T19:09:47Z

Relevant rust issue related to the naming of SocketAddr rust-lang/rust#16595

anguslees · 2015-02-16T05:18:21Z

text/0517-io-os-reform.md

+    fn send_to<A: ToSocketAddrs>(&self, buf: &[u8], addr: &A) -> io::Result<usize>;
+    fn socket_addr(&self) -> io::Result<SocketAddr>;
+    fn duplicate(&self) -> io::Result<UdpSocket>;
+}


Fwiw, there are useful reasons for calling connect() on a UDP socket:

a) In many protocols you are exchanging a stream of UDP packets with a single peer, and connect() can simplify your internal API.

b) In some (non-NAT-friendly) protocols (eg: RTP), you need to find a local IP address to tell the other end about. A reasonably common/portable/clean way to do this is to open a UDP socket, connect() to the peer IP to force the kernel to bind the local end, and then use getsockname() to fetch the IP address the kernel decided to use.

I agree! I believe that this should be done through a future Socket API though instead of having a large number of constructors on UdpSocket.

anguslees · 2015-02-16T05:32:59Z

If I may be so bold: By splitting it up into separate TCP client, server and UDP socket objects, I find this proposed API pre-supposes only UDP/TCP types of sockets and that the sockets will be used in particular ways. This places a number of restrictions on the underlying (assuming BSD sockets) API that don't exist at the syscall level. BSD sockets really just has different constructors, and then after that everything is a fd and you call (almost) the same set of syscalls on any socket. That underlying shared syscall structure isn't evident in this proposed API.
This would be fine, except it seems at odds with the other parts of the io revision RFC which offer composable building blocks and go to some length to have a 1:1 with low-level syscalls.

aturon · 2015-02-18T04:14:17Z

This PR has been belatedly merged (after the actual work had landed); it was left open by mistake. There's not much to debate here, given the very conservative design (and that it is largely just using the existing io traits). The renaming of SocketAddr is likely to land as a follow-up.

alexcrichton added 2 commits February 3, 2015 11:05

Add in std::net text from original RFC

0d9943e

Revise text with knowledge learned since proposed

9ba9437

alexcrichton self-assigned this Feb 3, 2015

codyps reviewed Feb 3, 2015
View reviewed changes

alexcrichton mentioned this pull request Feb 3, 2015

Separate Ipv4Addr/Ipv6Addr types, new methods for inspecting addresses rust-lang/rust#20785

Closed

aturon mentioned this pull request Feb 3, 2015

RFC: io and os reform: initial skeleton #517

Merged

Clarify that types are Send/Sync

cb10c25

Also remove some `&mut self` methods as only `&self` is necessary.

kjpgit reviewed Feb 3, 2015
View reviewed changes

l0kod reviewed Feb 5, 2015
View reviewed changes

seanmonstar mentioned this pull request Feb 6, 2015

feat(client): add connection timeout hyperium/hyper#295

Closed

alexcrichton mentioned this pull request Feb 6, 2015

std: Add a net module for TCP/UDP rust-lang/rust#22015

Merged

aturon mentioned this pull request Feb 6, 2015

Stabilization for 1.0-alpha2 rust-lang/rust#20761

Closed

38 tasks

glennpratt reviewed Feb 10, 2015
View reviewed changes

This was referenced Feb 11, 2015

UdpSocket is missing close_{read,write} rust-lang/rust#14926

Closed

std::io::net::unix::UnixStream does not support abstract sockets on Linux. rust-lang/rust#14194

Closed

steveklabnik mentioned this pull request Feb 14, 2015

TcpStream::close_read has platform-dependent semantics rust-lang/rust#19428

Closed

frewsxcv mentioned this pull request Feb 15, 2015

Rename SocketAddr to InetAddr rust-lang/rust#16595

Closed

anguslees reviewed Feb 16, 2015
View reviewed changes

alexcrichton mentioned this pull request Feb 16, 2015

Add bind_reusablemethod to UdpSocket rust-lang/rust#21480

Closed

aturon merged commit cb10c25 into rust-lang:master Feb 18, 2015

Centril added the A-net Proposals relating to networking. label Nov 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amend RFC 517: Add material on std::net #807

Amend RFC 517: Add material on std::net #807

alexcrichton commented Feb 3, 2015

codyps Feb 3, 2015

alexcrichton Feb 3, 2015

kjpgit commented Feb 3, 2015

alexcrichton commented Feb 3, 2015

kjpgit Feb 3, 2015

nagisa commented Feb 4, 2015

alexcrichton commented Feb 4, 2015

l0kod Feb 5, 2015

alexcrichton Feb 6, 2015

alexcrichton Feb 6, 2015

l0kod Feb 7, 2015

l0kod commented Feb 5, 2015

nagisa commented Feb 6, 2015

l0kod commented Feb 6, 2015

alexcrichton commented Feb 6, 2015

glennpratt Feb 10, 2015

frewsxcv commented Feb 15, 2015

anguslees Feb 16, 2015

alexcrichton Feb 16, 2015

anguslees commented Feb 16, 2015

aturon commented Feb 18, 2015


		Some important points of note are:

		* The `send` and `recv` function take `&self` instead of `&mut self` to indicate

Amend RFC 517: Add material on std::net #807

Amend RFC 517: Add material on std::net #807

Conversation

alexcrichton commented Feb 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjpgit commented Feb 3, 2015

alexcrichton commented Feb 3, 2015

Choose a reason for hiding this comment

nagisa commented Feb 4, 2015

alexcrichton commented Feb 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kod commented Feb 5, 2015

nagisa commented Feb 6, 2015

l0kod commented Feb 6, 2015

alexcrichton commented Feb 6, 2015

Choose a reason for hiding this comment

frewsxcv commented Feb 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anguslees commented Feb 16, 2015

aturon commented Feb 18, 2015