Support graceful shutdown on "auto conn"

[davidpdrsn]

This PR aims to unblock axum adding some kind of graceful shutdown convenience API. A somewhat common complaint with axum 0.7 is that graceful shutdown is more complicated than it was with hyper::Server.

axum provides a basic serve function that needs to support both http1 and http2. Therefore we use hyper-utils "auto conn" but that doesn't support graceful shutdown.

This PR aims to implement a minimal way to unblock that without having write a whole new server type:

• Instead of hyper_util::server::conn::auto::Builder::serve_connection_with_upgrades being an async fn that returns an opaque future, return something that holds the underlying Connection (and implements Future)
• That would be some type with an inner enum that either contains an http1::Connection or an http2::Connection
• A pub fn graceful_shutdown(self: Pin<&mut Self>) method that delegates to the inner Connection

I have it working for hyper_util::server::conn::auto::Builder::serve_connection but I don't think serve_connection_with_upgrades is possible since hyper's UpgradeableConnection type isn't nameable 😞 This is something I suspect we'll run into as well if we were to add something akin to hyper::Server to this crate.

@seanmonstar What do you think about this approach? Do you see hyper exposing UpgradeableConnection? It not then do you have any other suggestions?

[davidpdrsn]

Had to remove all references from this type since we can't use async/await anymore because we need a nameable Connection type to add the graceful_shutdown method to.

[mitioshi]

Shouldn't stuff like this be handled by axum? (e.g. as a layer, or as a part of the axum::serve::Serve struct). I believe the api would be simpler that way rather than handling graceful shutdowns on the hyper side

[davidpdrsn]

Shouldn't stuff like this be handled by axum? (e.g. as a layer, or as a part of the axum::serve::Serve struct). I believe the api would be simpler that way rather than handling graceful shutdowns on the hyper side

No. axum uses hyper-util's auto conn internally and it currently doesn't expose a way to call graceful_shutdown on the connection. So there is no way to axum to do it.

[davidpdrsn]

I've made it work now using hyperium/hyper#3457.

[GlenDC]

LGTM, look forward to be able to use this.
For now the auto connection is the only which I cannot gracefully shut down yet.
https://github.com/plabayo/rama/blob/1f437933e5abbb90b2cc3d903b118e1309d62bda/src/http/server/hyper_conn.rs#L172-L174

Look forward to be able to use this.

I'm by the way surprised that it's even allowed to use a private type in a public API. I know there's a clippy rule for it that does not allow it, but of course that can be seen as an opinion. It's not a compiler rule. I do get why you do it. In case you do not want people to rely on the return type there is not really another ergonomic solution, as an impl Trait return would only work if people also import the trait type in the callee file, otherwise they won't be able to use the methods.

So I guess using a private type in a public API is the only ergonomic choice you currently have if you anyway do not want to have people rely on the type and thus break their API, which I guess is what you want in a 1.0 release.. Then again, not sure how much of an issue that is, as anyhow people can already rely on its methods, so you either way need to continue to implement the same API you already have, even if you swap underlying type. In that context, you might as well expose the actual struct type already, as you can just as easily update that struct implementation (while keeping the API compatible) in the same way that you would swap types and respect that API.

As such I conclude that I remain confused about the choice of not exposing that type in the first place (UpgradableConnection) that is.

[detro]

@davidpdrsn I must say, I find this contribution like "an unexpected hug from a stranger". I don't know if this is a direct or indirect follow up of tokio-rs/axum#2355 (comment), but I appreciate this effort nevertheless.

[davidpdrsn]

@detro Thank you. Happy to hear 😊 I'm getting pretty exhausted with all the people asking about graceful shutdown in axum 0.7 and I really wanna improve the situation for people.

[detro]

@detro Thank you. Happy to hear 😊 I'm getting pretty exhausted with all the people asking about graceful shutdown in axum 0.7 and I really wanna improve the situation for people.

The internet can be a dark scary place sometimes, and it's easy to only complain when we don't like things, but say nothing when we do.

When I can, I like to highlight great example of human genuine help.

[okkero]

Happy to see so much effort being put into this 👍

[seanmonstar]

Looks good to me, just needs to be updated to point hyper at v1.1.

[davidpdrsn]

@seanmonstar Done!