Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make <Serve as Future>::Output = ! #3164

Open
jplatte opened this issue Jan 9, 2025 · 2 comments
Open

Make <Serve as Future>::Output = ! #3164

jplatte opened this issue Jan 9, 2025 · 2 comments
Labels
Milestone

Comments

@jplatte
Copy link
Member

jplatte commented Jan 9, 2025

See #2895. The PR conflicted with other ongoing work so it was not merged, but the idea was good and I think we should revisit it for the next breaking-change release. (unfortunately forgot to do it for 0.8)

@jplatte jplatte added the A-axum label Jan 9, 2025
@jplatte jplatte added this to the 0.9 milestone Jan 9, 2025
@jplatte
Copy link
Member Author

jplatte commented Jan 9, 2025

@SabrinaJewson I guess we could have merged the docs aspect of the PR / still merge it now. Do you want to open a new PR for that?

@SabrinaJewson
Copy link
Contributor

Right, PR sent.

In writing that PR, a thought came to mind: currently, in with_graceful_shutdown we have F: Future<Output = ()>. It seems reasonable that we might want to have F: Future, and the return value of WithGracefulShutdown’s future be F::Output. There is a subtle change in behaviour in that we would be forced to propagate instead of swallow panics, but I think that’s closer to what the user desires anyway.

Another thought that comes to mind is that it’s a performance hazard to run axum::serve on the main thread – i.e. inside #[tokio::main] – which I believe is because (a) it’s slower to spawn a task from the main thread than a worker thread (as the main thread doesn’t have access to a local queue) and (b) it’s possible having more active threads than cores is bad for performance.

Which is to say, I could envisage an API that looks more like this:

let server = axum::serve(listener, router);
tokio::signal::ctrl_c().await?;
server.graceful_shutdown().await;

Of course, Serve could still implement IntoFuture<Output = !> for the case when no graceful shutdown is needed, though that implementation would just have Future = Pending<!> (well, I guess it would be Pending<Infallible> in practice). Like before, if Serve is dropped then new connections would no longer be accepted while old connections would continue to run.

There is a drawback to this API, however, which is that axum::serve is now eager, meaning that we can no longer extend it with methods that control aspects of the serving. I don’t know how much we want this door to be left open. Of course, we could always use a builder:

axum::serve_builder(listener, router)
    .set_some_config(some_value)
    .build()
    .await

Or alternatively, have a .start() method returning a Handle type:

// option A:
axum::serve(listener, router).await

// option B, for graceful shutdown:
let server = axum::serve(listener, router).start();
tokio::signal::ctrl_c().await?;
server.graceful_shutdown().await;

One feature of this API is that Serve, being just a handle, could implement Clone, thus allowing things like graceful shutdown being triggered and waited on remotely without extra effort. I don’t think this is much of a motivating case, but it’s interesting to note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants