Add a `SocketBuilder` in `std::net`?

mathstuf · April 15, 2022, 12:29pm

This goes back to my previous question: if you get back ENOTSUPP or EINVAL (heck, ENOSYS might even be possible with some creative eBPF syscall filters installed), how do you know where to go and revise your calls? I'm sure we could make a proper error enum that has a variant per method to have that communicated out of this finalization method, but that seems…excessive.

keepsimple1 · April 15, 2022, 7:10pm

Thanks for the helpful comments! I've updated the APIs to:

Change name from SocketBuilder to UnboundSocket.
Add AsFd implementation. (for AsSocket on Windows, will need more time.)
Add #[non_exhaustive] for enums.
Try out a single new() method.
Try out defining a new SocketType and use it for new() method.
Add get_socket_type() method for sys_common UnboundSocket.

a new file: library/std/src/net/unbound_socket.rs :

/// A platform-independent wrapper for a socket that allows:
///
/// - socket configurations before binding to an address.
/// - convert into a `UdpSocket` or `TcpStream` or `TcpListener`.
pub struct UnboundSocket {
    inner: net_imp::UnboundSocket,
}

impl UnboundSocket {
    /// Creates a new UnboundSocket.
    pub fn new(addr_family: SocketAddrFamily, sock_type: SocketType) -> io::Result<Self>;

    /// Set "reuseaddr" to true or false. This is an example of possible configurations.
    pub fn set_reuseaddr(self, enable: bool) -> io::Result<Self>;

    /// The following methods convert UnboundSocket to a bound UDP or TCP socket.
    /// `SocketType` will be checked to make sure it matches.
    pub fn bind_udp(self, addr: &SocketAddr) -> io::Result<UdpSocket>;
    pub fn connect_tcp(self, addr: &SocketAddr) -> io::Result<TcpStream>;
    pub fn listen_tcp(self, addr: &SocketAddr) -> io::Result<TcpListener>;
}

/// The following methods are needed to support `AsFd`.

impl AsInner<net_imp::UnboundSocket> for UnboundSocket {
    fn as_inner(&self) -> &net_imp::UnboundSocket {
        &self.inner
    }
}

impl FromInner<net_imp::UnboundSocket> for UnboundSocket {
    fn from_inner(inner: net_imp::UnboundSocket) -> UnboundSocket {
        UnboundSocket { inner }
    }
}

impl IntoInner<net_imp::UnboundSocket> for UnboundSocket {
    fn into_inner(self) -> net_imp::UnboundSocket {
        self.inner
    }
}

impl fmt::Debug for UnboundSocket {
    fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result;
}

In library/std/src/sys_common/net.rs :
(note we also implement helper functions for SocketType here)

/// This is the `net_imp::UnboundSocket` used earlier.
/// 
/// This struct provides a bridge between platform-independent `UnboundSocket` 
/// and platform-dependent implementations.
pub struct UnboundSocket {
    /// This socket is platform-dependent.
    inner: Socket,
}

impl UnboundSocket {
    /// The actual implementation of methods.

    pub fn new(addr_family: SocketAddrFamily, sock_type: SocketType) -> io::Result<Self>;
    pub fn socket(&self) -> &Socket;
    pub fn into_socket(self) -> Socket;
    pub fn get_socket_type(&self) -> io::Result<SocketType>;
    pub fn set_reuseaddr(&self, enable: bool) -> io::Result<()>;
    pub fn bind_udp(self, addr: &SocketAddr) -> io::Result<UdpSocket>;
    pub fn connect_tcp(self, addr: &SocketAddr) -> io::Result<TcpStream>;
    pub fn listen_tcp(self, addr: &SocketAddr) -> io::Result<TcpListener>;
}

impl FromInner<Socket> for UnboundSocket {
    fn from_inner(socket: Socket) -> UnboundSocket {
        UnboundSocket { inner: socket }
    }
}

impl SocketType {
    pub fn to_c_int(&self) -> c_int;
    pub fn from_c_int(c: c_int) -> io::Result<SocketType>;
}

in library/std/src/net/addr.rs :

/// Address family values for sockets. This is useful to create a socket without a concrete address.
///
#[non_exhaustive]
pub enum SocketAddrFamily {
    /// Address family of IPv4.
    InetV4,
    /// Address family of IPv6.
    InetV6,
}

impl SocketAddrFamily {
    pub fn from_addr(addr: &SocketAddr) -> SocketAddrFamily;
}

#[non_exhaustive]
pub enum SocketType {
    /// Connection oriented, e.g. TCP.
    SockStream,
    /// Datagram oriented, connection-less, e.g. UDP
    SockDgram,
}

in library/std/src/sys/unix/net.rs , we change the signature of internal Socket::new to use the new SocketAddrFamily so that we can create a socket without a concrete address.

impl Socket {
    pub fn new(addr_family: SocketAddrFamily, ty: c_int) -> io::Result<Socket>;
}

All the existing callers to use SocketAddrFamily::from_addr() to convert from an address.

add AsFd for UnboundSocket in library/std/src/os/fd/owned.rs: (issue number is a fake one)

#[unstable(feature = "io_safety", issue = "99999")]
impl AsFd for crate::net::UnboundSocket {
    #[inline]
    fn as_fd(&self) -> BorrowedFd<'_> {
        self.as_inner().socket().as_fd()
    }
}

#[unstable(feature = "io_safety", issue = "99999")]
impl From<crate::net::UnboundSocket> for OwnedFd {
    #[inline]
    fn from(socket: crate::net::UnboundSocket) -> OwnedFd {
        socket.into_inner().into_socket().into_inner().into_inner().into()
    }
}

#[unstable(feature = "io_safety", issue = "99999")]
impl From<OwnedFd> for crate::net::UnboundSocket {
    #[inline]
    fn from(owned_fd: OwnedFd) -> Self {
        Self::from_inner(FromInner::from_inner(FromInner::from_inner(FromInner::from_inner(
            owned_fd,
        ))))
    }
}

===

Thanks!

keepsimple1 · April 19, 2022, 8:12pm

I wanted to clarify a couple of things:

My goal is to add support of socket configurations in std::net before UdpSocket::bind() (and similar calls for TCP).
If the goal makes sense, then the next / current step is to figure out what the API should look like. In the latest code listing, I opted to use a single new() as we are adding SocketType support. This is my first time trying to work on Rust stdlib, so any comments or inputs are helpful to me and appreciated. Thanks!

keepsimple1 · May 19, 2022, 7:17pm

Just wanted to check with this forum, should I continue to work on this proposal? Will it be a waste of time in your opinion, or should I try in a different venue?

I'm a newbie in contributing to stdlib, any suggestions are appreciated. Thanks a lot.

ckaran · May 19, 2022, 9:01pm

I like it! I suspect it will take a long time to get the API exactly right because everyone here has an opinion on what the 'right' API is.

Speaking about opinions... a trick that I just learned is that builders don't always need to return Self, they can return other builder types. So, you could have something like the following:

pub struct UdpBuilder{
    // Stuff
}

impl UdpBuilder {
    pub fn new() -> Result<SocketBuilder, Error> {todo!()}
}

pub struct TcpBuilder{
    // Stuff
}

impl TcpBuilder {
    pub fn new() -> Result<SocketBuilder, Error> {todo!()}
}

pub struct SocketBuilder {
    // Stuff
}

impl SocketBuilder {
   pub fn some_setting(self) -> Result<NextStageBuilder, Error> {todo!()}
}

// Etc., etc. etc.

You can only create a SocketBuilder from a UdpBuilder or TcpBuilder first, etc. That helps guide people to the head of the graph of builders. In addition, because you've got different types for each of the builder types, you have fewer places where errors can happen; that is, it is easily conceivable that the types of builders that UdpBuilder can produce are different from the ones that TcpBuilder can produce, which means that you don't even have the option of setting 'bad' options in certain cases; the type system and compiler prevent it from happening.

Note while you can create cycles in your builder graphs, I suspect that would be a bad idea, and that it would be best to ensure that you always have a DAG.

keepsimple1 · May 19, 2022, 10:30pm

Thank you! You inspired me to make some changes. Here is the high level API for UDP:

In std/src/net/udp.rs, we define a new type UnboundUdpSocket which provides UDP specific methods for an UnboundSocket. The underlying UnboundSocket is not public to the end user.

pub struct UnboundUdpSocket {
    inner: net_imp::UnboundSocket, // this can be refactored.
}

impl UnboundUdpSocket {
    pub fn new(addr_family: SocketAddrFamily) -> io::Result<Self>;
    pub fn set_reuseaddr(&self, enable: bool) -> io::Result<()>;
    pub fn bind(self, addr: &SocketAddr) -> io::Result<UdpSocket>;
}

then the user will be able to do this:

let unbound_udp = UnboundUdpSocket::new(SocketAddrFamily::InetV4)?;
unbound_udp.set_reuseaddr(true)?;  // and any other supported operations.
let udp_sock = unbound_udp.bind(&my_addr)?;
// Then, continue to do whatever with existing UdpSocket type.

(p.s. alternatively we can even define UnboundUdpSocket::new as private, and add a method like new_unbound() in UdpSocket type. But for now I will stay with a less involved definition as above).

And for TCP, it will be a separate UnboundTcpSocket. Like you said, such approach will reduce the error checking surface.

ckaran · May 20, 2022, 11:47am

Looks good to me! You can make as many builders as you want, the trick is balancing the ability to prevent errors at compile time vs. the number of builder types you eventually have. I think your next step will be to start fleshing out the API as you think makes sense, creating a new crate to hold it all. Once you have the new crate, you can link to it from here for others to review and collaborate on (you're rapidly going to hit the point where you have so much code that you can't just show ideas on a discussion board such as this one). Keep going, I like where you're going!

keepsimple1 · May 21, 2022, 1:26am

Thanks! Since this is a proposal to enhance the std lib, I think it won't be a new crate, right? I will try to flesh out the API and either post a new thread in this forum or in Rust's GitHub.

CAD97 · May 21, 2022, 3:47am

If it can be implemented externally (e.g. like/using socket2), it should be initially, for the purpose of testing/proving the API. If the design is then deemed suitable for Rust std, it can make the transition upstream.

Most larger library (i.e. not just a couple of functions) additions take this path, e.g. the provide_any RFC and dyno).

keepsimple1 · May 21, 2022, 9:36pm

I have to disagree. If we implemented this externally, it will be just another socket2 (or `net2). That defeats my goal.

Something like socket2 has to (re)invent its own Socket type and pretty much everything else as well.

I wanted to build on top of the existing std::net as much as possible. I don't want to replace std::net. To me, the first thing obvious is to enhance std::net to support socket configurations before binding the socket.

CAD97 · May 22, 2022, 3:56am

I agree, but prototyping it externally will still be beneficial to prove the API.

ckaran · May 23, 2022, 11:21am

@keepsimple1, I'm in agreement with @CAD97 here. You need to prototype it in an external crate first. std is such an important part of the ecosystem that once things are added to it, even experimental things, people start to treat them as if they are fixed and won't go away. I've been guilty of doing that a few times myself, and I've gotten burned because of it. What you're proposing is great, but it will take some time to get the API exactly right... and that's how 'right' it really needs to be before it can be made a part of std.

We're not trying to prevent what you're doing from being added to std, we're just trying to make sure it's a perfect fit for std before it gets added.

keepsimple1 · May 23, 2022, 3:38pm

@CAD97 @ckaran I understand and agree your point in principle. It makes sense when a new feature is self-contained, i.e. not relying much on existing std private code.

However, what should we do if the new API wants to use existing std private code heavily? In this case, for example, Socket type in platform dependent std::sys::net (Unix, Windows) and setsockopt in std::sys_common::net . Both of them are not exposed outside of std. I might missed something, but is there a way to build on top of them in a separate crate outside std? (If yes, that will be great!)

mathstuf · May 23, 2022, 4:21pm

Is there any reason that copying the code is not suitable? libc has setsockopt at least as well.

ckaran · May 23, 2022, 4:31pm

You're really not going to be able to do so, at least, not initially. Your best bet is to use libc in the back end completely hiding its API from your own public API, and getting things working that way. This will let you design your crate and get the API perfect. Once the public API is 100% perfect, you can ask to contribute it. Since the API doesn't expose libc in any way^[1] you can modify the backend code to use private code within std without affecting your public API.

I'm not in a position to review your API, but if I was and you exposed any API that wasn't defined by your crate or by std, I'd reject it on the spot. ↩︎

bascule · May 23, 2022, 5:06pm

I guess I'm the one dissenting voice here besides yourself: net2 and socket2 are ample precedent for adding such a feature to std, and with the entire point being interop with the existing std types in ways that can't be achieved via a third party crate, requiring deep integrations in private code, I don't see much point in developing it as a 3rd party crate.

This seems like a libs addition to std that doesn't need to go through the RFC process. I'd suggest just spiking it out in the rustc tree and opening a PR. Worst case you'll be told to write an RFC.

The API may be imperfect, but can remain nightly-only for awhile and be iterated upon.

keepsimple1 · June 3, 2022, 5:20am

Thank you for your encouragement. I finally decided to be brave and open a PR for this .

system · September 1, 2022, 5:20am

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.

Topic		Replies	Views
Pre-RFC: std::net expansion/refinement libs	13	4179	March 25, 2019
[feature_request] ICMP socket in std::net libs	3	519	August 31, 2023
Deprecate std::net?	10	1667	April 21, 2021
Nursery nomination: unix_socket libs	3	1910	March 25, 2019
Allow UdpSocket to set socket options before bind libs	5	243	September 22, 2024

Add a `SocketBuilder` in `std::net`?

Related topics