Make a GracefulShutdown helper in hyper-util

[seanmonstar]

With the removal of hyper::Server, the graceful shutdown stuff would disappear. We want to continue to provide a helper to do that, in hyper-util, but make it also able to be used with timeouts. Perhaps something like this:

let listener = TcpListener::bind("0.0.0.0:0").await?;

let server = Server::new();

let graceful = GracefulShutdown::new();
let ctrl_c = tokio::signal::ctrl_c();

loop {
    tokio::select! {
        conn = listener.accept() => {
            let server = server.clone();
            let graceful = graceful.clone();
            task::spawn(async move {
                // maybe TLS accept here...

                let conn = server.serve_connection(conn);
                let conn = graceful.watch(conn);

                if let Err(e) = conn.await {
                    eprintln!("http server conn error: {}", e);
                }
            });
        },

        _ = ctrl_c => {
            eprintln!("Ctrl-C received, starting shutdown");
                break;
        }
    }
}

tokio::select! {
    _ = graceful.shutdown() => {
        eprintln!("Gracefully shutdown!");
    },
    _ = tokio::time::sleep(10.seconds()) => {
        eprintln!("Waited 10 seconds for graceful shutdown, aborting...");
    }
}

[jacob-pro]

Hi @seanmonstar

I previously had a go at writing an implementation of this here:
https://github.com/jacob-pro/hyper-graceful/blob/master/examples/example.rs

One issue I struggled with doing this in a seperate crate is that I need to call UpgradeableConnection::graceful_shutdown when UpgradeableConnection is private - I wonder if graceful_shutdown needs to be a public trait instead?

For now I worked around it by using a closure so the user has to call graceful_shutdown directly themselves.

My other suggestion / question was about building a more general purpose "Connection Manager" which could have features similar to http://nginx.org/en/docs/http/ngx_http_limit_conn_module.html ?

[jfreedman0212]

Hey! Not sure if this is still an outstanding issue, but I can take a stab at it. I've implemented something like this with tokio's JoinSet before (docs). Would that be interesting?

I can whip up a little code example later, not at my computer right now.

[seanmonstar]

It is still outstanding :)

I admit I'm less familiar with JoinSet, what would it bring to the table? It would probably be nice if this util didn't need to be tied to Tokio. I think the usage pattern shown in the issue description is probably still a good thing to aim for, which allows people to spawn the tasks in whatever thing they want, after wrapping the connection in some watcher. I'd imagine the watcher itself is just an impl Future.

[jfreedman0212]

I was thinking that it would be easier to track the completion of the tasks instead of the connections themselves, which JoinSet is suited for. However, I didn't realize the connections themselves have graceful_shutdown functions (even though it's mentioned in this thread 😅), so JoinSet doesn't add anything that isn't already there.

I agree with you on keeping it runtime-agnostic and using the usage pattern you mentioned. To that end, I think the example @jacob-pro provided is a step in the right direction. The issue with it is that connections aren't shutdown by the ConnectionManager, they are instead shutdown by a closure the caller provides. Like they said, that could be fixed by having hyper expose a trait that wraps around the graceful_shutdown function on both http1::Connection and http2::Connection.

Alternatively, maybe having a GracefulShutdown implementation for both HTTP1 and HTTP2 separately could work? I've played around with it a bit, but I'm having a hard time getting the types to check out for passing an http1::Connection as itself (and not just a generic Future like in Jacob's example).

Are either of these options going down the right path? I feel like I'm spinning my wheels trying to get something to work following the usage you laid out. I'm probably just missing something

[GlenDC]

@seanmonstar can I pick this up together with the issue to add a Server to hyper-util?

#3436

[seanmonstar]

I think the two items are probably worth being separate. The Server one has some more design to work out.

[GlenDC]

Sorry @seanmonstar and @dswij for the confusion. Deleted the repo attached to the original PR not realising the PR was attached to it. It is now recreated as-is in hyperium/hyper-util#108