Fix incorrect rustdoc for TcpStream::connect

[kezhuw]

It is Interest::WRITABLE we should care about after non-blocking TcpStream::connect.

I find two manuals:

• https://man7.org/linux/man-pages/man2/connect.2.html EINPROGRESS

  The socket is nonblocking and the connection cannot be
  completed immediately. (UNIX domain sockets failed with
  EAGAIN instead.) It is possible to select(2) or poll(2)
  for completion by selecting the socket for writing. After
  select(2) indicates writability, use getsockopt(2) to read
  the SO_ERROR option at level SOL_SOCKET to determine
  whether connect() completed successfully (SO_ERROR is
  zero) or unsuccessfully (SO_ERROR is one of the usual
  error codes listed here, explaining the reason for the
  failure).

• https://www.freebsd.org/cgi/man.cgi?connect EINPROGRESS

  The socket is non-blocking and the connection cannot be completed immediately. It is possible to select(2) for completion by selecting the socket for writing.

I find two code usages:

• TcpStream::connect_mio from tokio.

    pub(crate) async fn connect_mio(sys: mio::net::TcpStream) -> io::Result<TcpStream> {
        let stream = TcpStream::new(sys)?;
  
        // Once we've connected, wait for the stream to be writable as
        // that's when the actual connection has been initiated. Once we're
        // writable we check for `take_socket_error` to see if the connect
        // actually hit an error or not.
        //
        // If all that succeeded then we ship everything on up.
        poll_fn(|cx| stream.io.registration().poll_write_ready(cx)).await?;
  
        if let Some(e) = stream.io.take_error()? {
            return Err(e);
        }
  
        Ok(stream)
    }

• uv__tcp_connect from libuv.

  uv__req_init(handle->loop, req, UV_CONNECT);
  req->cb = cb;
  req->handle = (uv_stream_t*) handle;
  QUEUE_INIT(&req->queue);
  handle->connect_req = req;
  
  uv__io_start(handle->loop, &handle->io_watcher, POLLOUT);
  
  if (handle->delayed_error)
    uv__io_feed(handle->loop, &handle->io_watcher);

[Thomasdezeeuw]

Well that's a major f*ck up on my part. This should 100% be writable, I luckily did do it correctly in the code.

[Thomasdezeeuw]

Thanks for catching this @kezhuw

[Andrepuel]

Hi, I've been playing around with TcpStream::connect and it seems that the documentation is not yet accurate.

The doc says "go back to step 3" if you get NotConnected error, i.e. wait for writable event again. I've been experiment with refused connections and we never get another writable event and the error is always NotConnected if I keep on polling.

It doesn't look like the code TcpStream::connect_mio does the "go back to step 3" logic as well.

It seems that what tokio does is wait for writable event and then propagate any kind of error. Without regarding what kind of error is that. I did something similar on my code:

fn poll_open(
    self: &mut mio::net::TcpStream,
    token: Token,
    polled_write: &mut bool,
    cx: &mut Context<'_>,
    manager: &PollManagerInner,
) -> Poll<io::Result<()>> {
    match *polled_write {
        true => Poll::Ready(self.peer_addr().map(|_| ())),
        false => {
            manager.pending(token, ObserveInterest::Write, cx);
            *polled_write = true;
            Poll::Pending
        }
    }
}

This seems to be working fine on my limited amount of tests (only tested only linux).

[Thomasdezeeuw]

If I recall correctly, it's been a while since I wrote this code, errors returned by connect(2) should be considered fatal. If connect(2) succeeds, getpeername(2) should be used to determine if the connection is actually connected or not. If getpeername(2) returns EINPROGRESS it's in progress. Linux always returns EINPROGRESS, other Unixes can also return ENOTCONN (mapped to io::ErrorKind::NotConnected). However it's been a couple years since I wrote this code.

Here is a working implementation: https://github.com/Thomasdezeeuw/heph/blob/29b661563f180a3d7c251043596c045659fd8ed2/rt/src/net/tcp/stream.rs#L562-L622.

[Andrepuel]

I see now. The difference between my code the one in your library is that you use take_error() function.
Unless I am misunderstanding completely the instructions. I think we should add another step between the current step 3 and step 4 saying that any error present on take_error() should be returned. Do you agree?

Just for the record, I did some prints using my code, this is what I get on take_error() and on peer_addr():

Xis {
    take_error: Ok(
        None,
    ),
    peer_addr: Err(
        Os {
            code: 107,
            kind: NotConnected,
            message: "Transport endpoint is not connected",
        },
    ),
}
// Writable event happened here
Xis {
    take_error: Ok(
        Some(
            Os {
                code: 111,
                kind: ConnectionRefused,
                message: "Connection refused",
            },
        ),
    ),
    peer_addr: Err(
        Os {
            code: 107,
            kind: NotConnected,
            message: "Transport endpoint is not connected",
        },
    ),
}

Remarks: The ConnectionRefused error is exactly the expected result as I am testing refused connections.

[Thomasdezeeuw]

I see now. The difference between my code the one in your library is that you use take_error() function. Unless I am misunderstanding completely the instructions. I think we should add another step between the current step 3 and step 4 saying that any error present on take_error() should be returned. Do you agree?

Yes. Would you like to open a pr for that?