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

RFC: Expand the std::net module #1158

Closed
wants to merge 3 commits into from

Conversation

alexcrichton
Copy link
Member

Expand the surface area of std::net to bind more low-level interfaces and
provide more advanced customization and configuration of sockets.

Rendered

Expand the surface area of `std::net` to bind more low-level interfaces and
provide more advanced customization and configuration of sockets.
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 11, 2015
@alexcrichton alexcrichton self-assigned this Jun 11, 2015
@alexcrichton
Copy link
Member Author

///
/// This function directly corresponds to the bind(2) function on Windows
/// and Unix.
pub fn bind<T: ToSocketAddrs>(&self, addr: T) -> io::Result<&TcpBuilder>;
Copy link
Member

Choose a reason for hiding this comment

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

It should probably be noted in the docs that using this in a builder-like fashion isn't possible in a non-painful fashion now (due to the io::Result wrapper), but may be in the future with something like ? sugar.

EDIT: ah, mentioned below

diagram, so this can be a natural conclusion to come to.

There are, however, not that many usages of typestate throughout the rest of the
standard library today, and there are a number of ergonomic and API concerns
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may deserve a more specific treatment. What are the ergonomic and API concerns? Could they be somehow worked around?

Sockets have exceptional familiarity in network programming, so the motivation to choose a different option shouldn't be too hand-wavy.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was already a somewhat quite lengthy alternatives section, so I didn't want to dedicate too much text to this. It's also very difficult to say precisely what difficulties there are without talking about an exact API as there are multiple methods of representing typestate. Do you have a specific API in mind that you would like to see?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I'd be very interested to see what this would look like. Contrary to what you said about ergonomic and API concerns, I think that providing a C-like level of access with added type safety preventing misuse at compile time is entirely compatible with what Rust is aiming for. If the standard library doesn't use it much yet in public APIs, it's only because there hasn't been much need (where else do you envision it being useful that isn't already taken care of by destructors)? I also don't think we should worry overmuch about overlap with the existing interface; Java shipping nio didn't cause the end of the world, and it's still early enough to deprecate the old stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without designing the whole thing, but to give a peek, it could look something like this:

struct Socket<Type> {
    // ...
}

enum Tcp {}
enum Udp {}

impl<T> Socket<T> {
    // any socket has these methods
    pub fn try_clone() {}
    pub fn shutdown() {}
}
impl Socket<Tcp> {
    pub fn accept(&mut self) -> io::Result<()> {
        // ...
    }
}

Usage then could be:

let mut sock = Socket::<Udp>::connect(addr);
let other = sock.try_clone().unwrap();

sock.accept(); // compile error, Socket<Udp> does not have a method `accept`

Copy link
Member Author

Choose a reason for hiding this comment

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

That form of typestate is actually basically equivalent to the TcpStream and UdpSocket distinction that we have today. It's unclear if you every really actually want to program with a Socket<T> vs a concrete Socket<Tcp> (e.g. Socket<T> can barely do anything).

@sfackler
Copy link
Member

It seems like the right way forward here is a bit less clear than in other API proposals. Fortunately, I believe that the proposed APIs are implementable outside of libstd via FromRaw{Fd,Handle}. How do people feel about implementing this in a separate crate initially? I think we'd want to treat it a bit differently than some of the other rust-lang repos in that we specifically intend to merge it back into libstd in the relatively near term.

Keeping it out of libstd will allow us to iterate more quickly, allow it to be used on the stable channel, and allow crates to use the functionality without bumping their minimum rustc version.

I'm not sure if we'd also want to expose the API as unstable in libstd via similar trickery as liblibc, but my gut would say no for simplicity unless there's a compelling reason to.

@birkenfeld
Copy link

Is there a plan to add something like gethostname()? Would be necessary to find out addresses of local interfaces...

@llogiq
Copy link
Contributor

llogiq commented Jun 17, 2015

@sfackler: Agreed, incubating this out of tree will probably lead to better design in the long run. We may even have different implementations until a 'winner' emerges.

to configure the default at a global level for Linux as well. Despite this, to
increase cross-platform interoperability, sockets created by `TcpListener::bind`
and `TcpStream::connect` will have this option set to `false` by default on *all
platforms*.
Copy link
Member

Choose a reason for hiding this comment

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

I have to say I find this worrisome. If I understand this paragraph properly, it says that Rust programs on Linux are going to actively ignore the default that an admin cared enough about to set it manually. I don't think admins will like this. Is there a precedent of another cross-platform language making a similar choice?

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears ruby does this by default at least, although there's definitely an interesting balance to draw here!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I find this worrisome as well. I do not think we should override defaults by default.

@TheNeikos
Copy link

With this proposal how would a timeout interact with accept?
Would it just be an Err with EAGAIN?

(This is partly related to the above issue I closed as it could solve the problem I explained.)

@sfackler
Copy link
Member

I don't believe any of this deals with support for accept timeouts.

@TheNeikos
Copy link

If you set a timeout on a socket on which you accept it will return after that time of inactivity, at least that is what I read it out of the man pages.

However, if I am wrong, which is quite possible, how would one then deal with the situation I showed?

EDIT:

I have now checked again(was on mobile before), and it seems that it should be possible.

The man page in question.
http://linux.die.net/man/3/setsockopt
https://www.freebsd.org/cgi/man.cgi?query=setsockopt&sektion=2

Although, if it is not possible I do not think that it is a good idea to go with a similar style as is present right now, that is with an iterator. Since there are only two ways to exit 'cleanly'.

  1. The socket errors out for some reason. Should not be expected (without a timeout that is)
  2. When a new connection is engage. Which also seems unclean to use a connecting client as an exit window.

@alexcrichton
Copy link
Member Author

@sfackler

How do people feel about implementing this in a separate crate initially?

I think prototyping this sort of functionality outside the standard library is a good idea, but I don't think that we should plan on leaving it out of the standard library indefinitely. This also I think extends the amount of time for this interface to become stable as once it's baked outside the standard library it'll also want to bake inside the standard library (as opposed to just baking inside for a bit).

I'll try to make a prototype library in the near future though and see how it turns out.


@birkenfeld

Is there a plan to add something like gethostname()? Would be necessary to find out addresses of local interfaces...

At this time, no, but that's certainly a future extension!


@TheNeikos

With this proposal how would a timeout interact with accept?

@sfackler is correct in that this RFC doesn't add an option for that. Unfortunately I don't believe there's a socket option corresponding to the timeout on an accept operation, but if there is one we could certainly bind it!

@seppo0010
Copy link

I like the proposal. I wonder if it would be possible and useful to break the API into traits, so people can support unix sockets easier receiving a generic type, e.g.: instead of TcpStream use T: Read + Write + TryClone + ReadTimeout + WriteTimeout, and instead of TcpBuilder T: Listen (maybe the builders methods can also be grouped in some way?).

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jul 1, 2015
@alexcrichton
Copy link
Member Author

This RFC is now entering its week-long final comment period.

@sfackler
Copy link
Member

sfackler commented Jul 1, 2015

I'm on board, and would prefer if it was implemented out of libstd initially to allow for broader use and faster iteration.

@alexcrichton
Copy link
Member Author

@sfackler, ah it slipped my mind but awhile back I implemented this RFC in the net2 crate on crates.io. To clarify, though, if this RFC is accepted would you want to hold off on the implementation in the standard library?

@sfackler
Copy link
Member

sfackler commented Jul 1, 2015

Oh, nice. Not sure about merging it into std as well - we would have to decide what we want to do about synchronizing it with net2. Maybe add a note in the std docs to direct people towards net2 and sync them before releases? It could also be the case that the current setup works fine and we won't need to make many changes.

@alexcrichton
Copy link
Member Author

The consensus of the libs team on this RFC is that the API is ok to merge, but we stalled out discussing what to do about whether this starts out as an external crate or in the standard library. We agreed on not having both of these as "officially maintained", so we just need to decide on one or another.

As a results this RFC is going to remain in its final comment period for another week.

@muja
Copy link

muja commented Jul 21, 2015

Is there any plan to expose a local address option for a TcpStream? This RFC seems to focus on listeners, and I'm not sure where else to ask, so sorry if this isn't the right place. The motivation of the question is here: hyperium/hyper#602

@alexcrichton
Copy link
Member Author

@muja Yes this should enable that by using TcpBuilder, calling bind, and then calling connect.

@seppo0010
Copy link

I don't know if my previous comment didn't make sense, so I'll insist 😀

I'd like to see the methods grouped into traits, because currently the way to reuse code in {Unix,Tcp}{Socket,Listener} is with macros, but generics would be nicer considering they are implementing the same API. Unless I'm missing something.

@alexcrichton
Copy link
Member Author

@seppo0010 oh sorry I missed that! Adding traits to abstract over these is a backwards compatible addition and unix sockets also don't exist in the standard library today. Like with collections we prefer to start out with concrete interfaces and then leave the door open to adding a trait to be generic in the future.

@muja
Copy link

muja commented Jul 24, 2015

Would it make sense to not make any IO actions until the final call to listen or connect and instead store the options locally? This way a TcpBuilder can be reused multiple times over, like this:

let builder = TcpBuilder::new_v4();
builder.bind("192.168.0.1:0");
let conn1 = try!(builder.connect("http://rust-lang.org:80"));
let conn2 = try!(builder.connect("http://github.com:80"));

Specifically this would be extremely useful in interplay with hyper, since it would allow using a TcpBuilder as a NetworkConnector.

The drawback obviously would be that more memory is needed and the errors will be delayed for the last step, but I think it's still worth it. Thoughts, opinions?

@alexcrichton
Copy link
Member Author

@muja unfortunately I think the drawback of delaying errors makes it not worth it because the precise knowledge of where a chain of operations failed is often necessary. For example if bind failed on one address there may be a fallback one to try, but you want to only try that if the bind itself failed, not the socket creation or connect itself.

@muja
Copy link

muja commented Jul 24, 2015

@alexcrichton isn't it possible to have different Errors and return to see where it failed?

@alexcrichton
Copy link
Member Author

It is, yes, but it can get pretty complicated pretty quickly. Plus if you only receive the error at the end you don't have the partial state you've built up so far. This means that in the case I described above, you'll actually create two sockets instead of one because the first is destroyed after the bind failed.

@muja
Copy link

muja commented Jul 24, 2015

Right, the 2-socket-argument makes sense.
Will it be possible to clone a TcpBuilder before a call to connect, basically creating a new socket with the same options to that point? If I want to make a multitude of connections with the same socket options, it would be extremely convenient not having to always call the functions again.

@alexcrichton
Copy link
Member Author

Unfortunately I don't think so because I believe the dup or WSADuplicateSocket syscalls don't create an independent socket but rather just a new reference to the existing one. You'll probably want to make a helper function which generates a TcpBuilder in a known state and that can be used as a "clone" operation.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jul 28, 2015
These methods are all covered by [RFC 1158] and are currently all available on
stable Rust via the [`net2` crate][net2] on crates.io. This commit does not
touch the timeout related functions as they're still waiting on `Duration` which
is unstable anyway, so punting in favor of the `net2` crate wouldn't buy much.

[RFC 1158]: rust-lang/rfcs#1158
[net2]: http://crates.io/crates/net2

Specifically, this commit deprecates:

* TcpStream::set_nodelay
* TcpStream::set_keepalive
* UdpSocket::set_broadcast
* UdpSocket::set_multicast_loop
* UdpSocket::join_multicast
* UdpSocket::set_multicast_time_to_live
* UdpSocket::set_time_to_live
bors added a commit to rust-lang/rust that referenced this pull request Jul 29, 2015
These methods are all covered by [RFC 1158] and are currently all available on
stable Rust via the [`net2` crate][net2] on crates.io. This commit does not
touch the timeout related functions as they're still waiting on `Duration` which
is unstable anyway, so punting in favor of the `net2` crate wouldn't buy much.

[RFC 1158]: rust-lang/rfcs#1158
[net2]: http://crates.io/crates/net2
@aturon aturon removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Aug 7, 2015
@alexcrichton
Copy link
Member Author

This RFC is now re-entering its week-long final comment period. In light of #1242 merging recently my personal feelings are to close this without merging. Our new policy is to develop the crate in the rust-lang-nursery organization for some time and then RFC upon reentry into the standard library, so having an RFC here is a bit premature (but perhaps groundwork for a future one!)

@alexcrichton
Copy link
Member Author

er, didn't mean to close

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Aug 27, 2015
@alexcrichton
Copy link
Member Author

The libs team discussed this RFC during triage today, and the conclusion was that following the new rust-lang crates process we're going to close this and let the net2 crate develop externally for awhile. It's likely to make its way into the nursery once that gets underway, however!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.