Hello everyone! I’ve written up a pre-RFC targeted at tackling many of the already reported issues. I’ve talked about this quite a bit with @aturon already and the alternatives section is much larger than usual, and I’m still personally somewhat on the fence about what direction to take on these APIs, so I’m curious as to others’ opinions about this!
cc @aturon, @sfackler, @Gankra, @burntsushi, @Kimundi, @huon, @brson
It's unclear why the TcpStream and other refined primitives still exist. This API is a union of the TcpStream, TcpListener, and UdpSocket APIs and poses a new question of "should I use a Socket or TcpStream?".
I'm not very keen on letting specific APIs stop us adding more general versions of those APIs, in general. In this specific case, it certainly seems fine and good to have the most common cases (i.e. the current std::net) have type-safe higher-level wrappers around the raw Socket API (which would try to support as many of the platform-supported socket types as possible). However, this is less convincing to me if there's not really any more socket types that Socket can support on any platforms (are there more?).
I’m wondering if there is anyone out where who has needed to dip into FFI to get access to things like raw sockets (or other net functionality not provided by today’s rust). If so, what was your experience? If this isn’t too badly painful, and is not often needed, does it make sense to suggest this as a solution? By this I mean force users to deploy their own “all powerful” solution as needed (especially if they only care about 1 platform)
I’d be very interested in making non-blocking reads, but only for detecting the status of the sockets.
@huon Unix sockets are another type.
Socket would be nice if for nothing else then as something to implement TcpStream, UdpStream etc as wrappers around to cut down on some code duplication. It wouldn’t necessarily be something users would ever interact with directly, but could be public to make third party crates like unix_socket a bit cleaner when possible.
I don’t think the setup of the builders returning io::Result<&Self> is workable. Builders APIs are structured to allow for nice method chains but this isn’t possible here unless we do something like add method macros:
try!(try!(try!(try!(TcpBuilder::new_v6())
.nodelay(true))
.keepalive(Some(Duration::milliseconds(500)))
.read_timeout(None))
You could also nest Result::map calls but it’s also pretty gross.
Some options:
io::Result<()>.&Self. The builder internally stores the last io::Result. If it’s an Err, any further calls are ignored, and the stored result is returned from calls to terminal methods like bind.FileOption. No calls are actually performed until a terminal method is reached when everything’s set up at once. It’s externally isomorphic to the previous option except that the builder can be Clone which is kind of nice.Chaining-with-results will be much nicer if we get postfix ?:
TcpBuilder::new_v6()?
.nodelay(true)?
.keepalive(Some(Duration::milliseconds(500))?
.read_timeout(None);
Yeah, I only ask the question because the text wasn't sure it would apply:
The scope of this API is somewhat unclear, for example is Socket intended to encompass Unix sockets as well? It attempts to via the
Into<RawSocketAddr>trait bound, but it's arguably not super ergonomic.
If we decide that we do like the higher-level types regardless of the lower level Socket, then we could probably just forget about the lower level version until/if there's a stronger need for it?
I agree. I think any of the three alternatives you proposed would be better. I am personally fond of 2 or 3.
From the RFC, RE lots of options that are hard to discover:
Another way to mitigate this would be to use a separate impl block for the options. rustdoc will show it separately in the produced docs, so that might help a little. I do it for csv::Reader. I'm not sure how well it works, and it's definitely a bit noisy.
If we’re expecting postfix ? to land in the nearish term we could keep the proposed -> io::Result<&Self> API but use examples to teach people to not try to chain calls. When ? lands we’ll get a “true” builder style workflow for free.
I definitely agree that I don't want the existing structures to prevent the addition of new ones, but I definitely feel that TcpStream and the other types have their place (e.g. going to only Socket seems a bit extreme). There are indeed other socket types, however:
Put another way the TcpStream type and friends definitely do not cover all possible sockets, so we're losing out on a bit of possible interoperability with these other kinds of sockets. I personally see that as OK as the other kinds of sockets would likely also want their own standalone type to prevent mixing them up, and the semantics are so radically different that putting them all into one (when binding the FFI apis yourself isn't so hard) may not be worth it at this time.
Two users that I'm aware of is @sfackler with unix-socket and @mrmonday with libpnet.
Making a nonblocking read should definitely be possible today, it's just a matter of flipping the flag on the file descriptor. We don't currently have a method for doing so, and for now I wanted to avoid adding this method until we have some more time to think through the async I/O story, but it's also a pretty minor addition that could come in at any time (unstable of course).
I'm not quite sure what you mean by detecting the status of a socket, I would personally have to investigate the semantics of a 0-length read.
I was possibly thinking that one day we could have std::os::Socket so it's at least in a separate location, but I was hoping to avoid it for now to stay conservative. I do think, though, that putting it in std::net (if we have builders) may be too confusing in the long haul.
Wow actually seeing that on paper definitely leads me to agree!
I'm somewhat hesitant to do this as it definitely detracts from "these are builders" as they're really only builders-by-name at that point and don't have any of the other ergonomic wins of builders elsewhere. For example the TcpBuilder basically wouldn't follow any builder conventions!
Hm, this isn't a bad idea. An important part about the API I wanted to preserve is precisely where an error happened (more on this below as well), but if we had a method to access the last error I think that may be ok.
The unfortunate part about this strategy is that you lose the knowledge about where the error happened. All our other builders don't actually do any I/O operations until the very end, and it's sometimes just one I/O operation, so the point of failure is always known. With TcpBuilder, however, each step is an I/O operation and precisely which one failed can sometimes be important.
@aturon was thinking we could possibly return some error payload information at the very end indicating which step failed, but I haven't thought through this much and am somewhat skeptical how it may pan out.
I definitely agree with this! I suspect though that this may still be far enough out we may not be able to wait on it
Good idea!
Thanks for the comments everyone! I'll have to think a bit on what to do about the builder aspect. I originally was going to propose std::net::Socket but the idea of a builder won me over, but this builder API definitely seems... unusual so far!
I’m kinda busy right now, so I haven’t had a chance to read through everything - I’ll give a quick overview of a few things, and if there’s anything more you wanna know ping me and I’ll see if I can answer.
In terms of what socket()can handle for layer < 7 networking (data link/network/transport):
AF_PACKET. This is not standard or portable - every other OS does this in a different and incompatible manner (ranging from custom system calls, to reading from a special /dev device), see also pnet::datalink::*.I would highly recommend against having anything related to low-level networking in the standard library. I’ve been working on the stuff on and off for about two years with Rust, and it’s still a long way from being perfect*. It’s also a fairly nuanced/specific area, which is probably better in its own library (<insert shameless plug for people wanting to help with libpnet here>).
In terms of a standard socket() API, I’d need to think about it some more. If there’s an idea for an API I’d be happy to look over it and see how well any low level stuff could integrate with it. I can’t think of a nice way to do it off the top of my head, but that doesn’t mean it can’t be done. What I imagine is that it will be hard to have any level of sensible type safety without essentially integrating large chunks of libpnet into the standard library.
Hope this helps, I’m happy to answer any questions you may have.
* Where perfect here means something you’d be happy marking as #[stable] in libstd.
Haven’t looked into the rest of the API, but it’s my opinion that builders in general are a prime example of design patterns as missing language features, and are especially dubious if supporting them requires designing the code in an awkward way - here, by requiring duplicating the setters on both builders and socket objects, and in option 2, by implicitly accumulating errors inside the object, both of which can be confusing.
That said, if the builder is to be just a bag of options and flags, changing it from an OO-style interface to just be a public struct with a Default implementation would chop off a good amount of boilerplate, and emphasize the difference between builders and sockets.
I wouldn't mind that either necessarily, but I don't think it's feasible here. I think an explicit design goal is to be able to easily add new socket options in the future. (Which can't work with structs with public members.)
Oh, right, it’s that issue I read about recently (on Reddit?) where functional record update no longer allows setting private fields, so what would previously have been a reasonable solution, adding a dummy private field with the intent that users use , ..Default::default(), is not sufficient to allow future expansion…
There really needs to be a language solution for that general use case, since it’s quite a useful pattern, though I suppose it shouldn’t hold this up.
This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.