Clean shutdown of TCP sockets

[P-E-Meunier]

I just implemented clean connection shutdown again in Thrussh (since I moved it to Tokio a few months ago).

The Tokio interface for that was not totally easy. Here are things I'd like to have:

• I needed to call .shutdown(Shutdown::Both) on TcpStream. This works as long as the underlying connection is indeed a TcpStream, but what if I want to run the connection in a buffer for testing? I wrote a trait called Tcp, containing a method shutdown that does that, but I guess there could be something more generic.

• There is a name conflict on shutdown methods in TcpStream and AsyncWrite. Not a big deal for me, but on a larger codebase, it might be burdensome. How about having the shutdown method of TcpStream not take self, but instead having to call tokio_core::net::TcpStream::shutdown(&mut stream)?

• Or there might be something more general: I don't know whether "TCP shutdown" should be called from within the AsyncWrite::shutdown method of TcpStream, because there might be a number of parameters to a proper TCP shutdown, in some cases. But I guess it is fairly standard to send a shutdown and waiting for a read to return 0, so why not have another type called CleanShutdownTcpStream whose Async::shutdown method does exactly that?

[alexcrichton]

Thanks for the report! I agree it's a little unfortunate the naming conflict between AsyncWrite::shutdown and TcpStream::shutdown, although you can already, today, use the method like TcpStream::shutdown(&stream) I think?

The implementation for TCP to not actually do anything on shutdown was actually pretty deliberate, AFAIK there's not really a well established canonical way to shutdown a TCP socket, I think it's mostly just "some applications do it, most probably dont"

[izderadicka]

Just wondering why implementation of shutdown for AsyncWrite for TcpStream is just dummy - this is I believe current implementtaion:

impl<'a> AsyncWrite for &'a TcpStream {
    fn shutdown(&mut self) -> Poll<(), io::Error> {
        Ok(().into())
    }

Why we cannot provide rather default implementation that is on proxy example - eg. call stream shutdown with Shutdown::Write? For proxy example new struct had to be created (worth app. 20 lines of code) just to implement this code.

My reasoning why to include default implementations is:

1. If you do not need it, you will not call it
2. If you need it then probably most common case is like in proxy example - you have upstream connection that ended and you need to close downstream connection
3. If you need anything else you can still override it as before

And indeed name conflict can be bit confusing.

[alexcrichton]

@izderadicka the intention of shutdown was for things like "shut down the TLS connection" or "flush remaining buffers and then shut down the underlying socket" or things like that, the name shutdown wasn't intended to convey a literal TCP shutdown and was a mistake on our part.

[carllerche]

Closing this issue in favor of #80.