Graceful client shutdown

[goatgoose]

Is your feature request related to a problem? Please describe.

It appears that the hyper-util client isn’t currently gracefully shutting down its connection with the server. Specifically, the hyper client seems to be calling shutdown on the client’s stream, and then immediately closing the socket without waiting for the server’s shutdown. This can result in a NotConnected socket error when the server attempts to send its shutdown to the client, since the socket is already closed.

Describe the solution you'd like

It would be useful to have a way to allow the hyper-util client to gracefully shutdown, in order to avoid issues like the NotConnected socket error emitting on the server. This could be in the form of a Client shutdown() API, which sends a shutdown and waits for an end-of-stream indication from the server, before returning control to the application and closing the socket.

Additional context

The NotConnected socket error seems to be raised due to a socket race condition where:

• The client sends its shutdown and immediately closes its socket.
• The server writes some data to the socket
• The server attempts to send its shutdown to the client, raising the NotConnected error.

If the server doesn’t write any data to the socket before sending its shutdown, a socket error isn’t raised.

I encountered this issue while testing the s2n-tls hyper-util connector crate. I believe this issue is immediately observable for TLS streams since TLS streams will always write a close notify alert when sending a shutdown. However, there may be ways that this issue could impact normal TCP streams as well, such as if the server is sending data with HTTP server push before shutting down.

To verify that this isn’t just an issue with s2n-tls, I reproduced the issue with rustls (modified from the hyper-rustls examples). However, it seems that rustls works around this issue by ignoring NotConnected socket errors in tokio-rustls (rustls/tokio-rustls#41). When I removed this suppression from poll_shutdown() in tokio-rustls, I observed the same NotConnected error that I’m seeing with s2n-tls-tokio. A possible solution to this problem could be to similarly ignore NotConnected errors in s2n-tls-tokio. However, it seems like it could be better for the hyper client to perform a graceful shutdown instead.

I was also able to reproduce this issue outside of a TLS stream by wrapping a TCP stream with a poll_shutdown method that writes some data to the socket before shutdown. See the following rust playground example:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d4baf4675d8af0a45d0c924a933c7812

[seanmonstar]

Interesting! What do you propose hyper would do? After poll_shutdown() is successful, wait for a poll_read() to return Ok(0)?

I think to get that to work with just TCP, the poll_shutdown() would need to actually call shutdown(Shutdown::Write), to tell the remote that nothing more is coming. Otherwise the connection would just linger as idle.

[goatgoose]

Hi @seanmonstar! Yes - I think hyper would ideally have a way to poll_shutdown() and then poll_read() for the server's end-of-stream. I'm not familiar with how shutdown currently works in hyper, but one way this could maybe work is a shutdown() API that applications could call. For example:

// Make a request to the server.
let client: Client<_, Empty<Bytes>> =
            Client::builder(TokioExecutor::new()).build(connector);
let response = client.get(uri).await?;

// Provide a new hyper_util::client::legacy::Client API which gracefully shuts down
// the client by calling poll_shutdown() and waiting for the server's end-of-stream
// with poll_read().
client.shutdown().await?;

Applications that don’t call the shutdown() API would continue to experience the current shutdown behavior.

I agree that TCP will need to actually initiate a shutdown in poll_shutdown() for this to work. It does seem that tokio performs an actual shutdown for TcpStream.

[seanmonstar]

I think I could be in favor of such a change (after poll_shutdown, wait for poll_read to return 0), with a possible option to enable or disable the waiting. I think an additional public method, while not a never, might be worth waiting to see if it's needed.

Would you be interested in doing the work here?

[goatgoose]

Ok great! That sounds good.

I'm definitely interested in doing this work, but I might not be able to prioritize it for a while. If this issue is still open when I can get to it, though, I'd be happy to work on it!