Connection::graceful_shutdown always waits for the first request.

[sfackler]

Version
hyper 0.14.16

Platform
Linux DESKTOP-DHO88R7 4.19.104-microsoft-standard #1 SMP Wed Feb 19 06:37:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Description
If you gracefully shut down a server connection future before the first request, Hyper will not actually shut the connection down until a request is processed:

use hyper::server::conn::Http;
use hyper::Response;
use tokio::io::AsyncReadExt;
use tokio::net::{TcpListener, TcpStream};
use tokio::pin;

#[tokio::main]
async fn main() {
    let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
    let addr = listener.local_addr().unwrap();

    tokio::spawn(server(listener));

    let mut stream = TcpStream::connect(addr).await.unwrap();
    println!("connected");
    let mut buf = vec![];
    stream.read_to_end(&mut buf).await.unwrap();
}

async fn server(listener: TcpListener) {
    let socket = listener.accept().await.unwrap().0;

    let service = hyper::service::service_fn(|_: hyper::Request<hyper::Body>| async {
        Err::<Response<hyper::Body>, _>("no")
    });

    let future = Http::new()
        .http1_only(true)
        .serve_connection(socket, service);
    pin!(future);
    future.as_mut().graceful_shutdown();

    future.await.unwrap();
}

I would expect this program to exit almost instantly since there is no request being processed when the graceful_shutdown is invoked. However, it instead blocks forever waiting on the client to send headers.

The behavior actually appears to be that the shutdown is processed immediately after the first request is fully handled.

[seanmonstar]

Yea, I think this is behavior is currently on purpose. Whether it should be is a fair question. I think at the time, I assumed that a new connection would usually have a request incoming ASAP, whereas an idle connection might not, and so it was better to allow that request to come in.

We could alter that behavior, if we document a good reason for why we're making that change. Also, I imagine using the http1_header_read_timeout option could help here.

[sfackler]

I agree that it's more likely for the first request to show up than a follow up request, but it seems weird to bake that assumption into graceful_shutdown IMO. For reference, I'm using graceful_shutdown in some logic to shut down connections after a certain period of idleness (I can't just terminate the connection future since I want the TLS session to shut down cleanly for session reuse). Right now, the current behavior will miss connections that are opened and never make a request.

Does http1_header_read_timeout start counting immediately or just once the first byte of the header is received? In any case, I don't think that'd help for an h2 connection, right?

[sawyersteven]

Any progress on this? I was trying to figure out why my program wasn't exiting when I expected it to and found this issue. If I send the graceful shutdown signal the server will process one more request before actually exiting. So a user sending the shutdown command through a web app won't actually be shutting anything down.

[bossmc]

Does http1_header_read_timeout start counting immediately or just once the first byte of the header is received? In any case, I don't think that'd help for an h2 connection, right?

From some testing just now, the answer to the first part is "no". If I start-up an http1::Connection with a header timeout configured and connect to it simply with nc then the connection stays up indefinitely. Once I send any data (even a blank line) the timer starts counting and the connection is closed as expected after the timeout.

[seanmonstar]

This seems like a thing to slide into RC2 or RC3.

I can't just terminate the connection future since I want the TLS session to shut down cleanly for session reuse

@sfackler Does this mean you'd have the connection write out a 408 response and then finish? Or simply calling poll_shutdown and then return the future like if it were in keep-alive state? I guess calling shutdown will make the TLS shutdown start, right?

[sfackler]

Yeah it just needs to call shutdown on the underlying IO object, which will handle the SSL shutdown protocol.

[rob2244]

I think I can take this one.

[rob2244]

@seanmonstar so the issue right now is this:

if we try to control this using the keep_alive state (which is what we're currently doing) it introduces a race condition of sorts.
If keep_alive is initially Idle, when disable_keep_alive() is called (either directly or through graceful_shutdown) the connection will shut down before servicing any requests. This seems like incorrect behavior, it breaks some unit tests and I would expect that the connection would respond to one request before shutting down.

If keep_alive is initially Busy then the connection will wait forever on the read of the initial request.

I think we have two options here: either we keep the initial state as Busy and add some sort of read timeout somewhere, or we keep the initial state as Idle and do a peek on the underlying io stream and process any data found there before closing the connection.

edit:
Actually I found another way to do it I think. Let me see if it works. Not sure I'm super happy with it thought because it feels a little like catering to an edge case too much

[seanmonstar]

If keep_alive is initially Idle, when disable_keep_alive() is called (either directly or through graceful_shutdown) the connection will shut down before servicing any requests.

That seems like the desired outcome of this issue, no? Or do you mean the request was already received, and the connection just aborts abruptly?

[rob2244]

@seanmonstar yeah so essentially if a http1 server conn is created with .keep_alive(false) the server shuts down without servicing any requests. Is that the behavior we want? I would think you would want the server to service one request to see if that request wants to keep the connection alive or not.

The following two tests fail when the initial keep alive state is set to idle:

The read_to_end in both cases returns an empty array because the server shut down before processing anything.