-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
More generally useful doc example for .with_graceful shutdown()
#2820
base: main
Are you sure you want to change the base?
Conversation
The current doc for with_graceful_shutdown() implies a select! block somewhere else that races the serve and the cancellation futures. I think using a `tokio::sync::Notify` makes for a more generally useful pattern since it can be passed around and triggered as needed. For example, the select! strategy is difficult to use in integration tests.
axum/src/serve.rs
Outdated
/// let cancel = Arc::clone(&blocker); | ||
/// tokio::spawn( | ||
/// axum::serve(listener, router) | ||
/// .with_graceful_shutdown(async move { blocker.notified().await }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple blocker.notified()
doesn't work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lifetime issue?
axum/src/serve.rs
Outdated
@@ -121,20 +121,23 @@ impl<M, S> Serve<M, S> { | |||
/// | |||
/// ``` | |||
/// use axum::{Router, routing::get}; | |||
/// use std::{future::IntoFuture, sync::Arc}; | |||
/// use tokio::sync::Notify; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, can't we use just a oneshot channel for the example? As long as users don't need to see how to cancel from multiple places the whole Arc
is unnecessary and it might be cleaner to have the semantics of being able to shutdown only once and having a dedicated side for producing and consuming the notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The oneshot channel is fallible, so it has some noise too, but I still agree that it becomes a bit clearer that way. Updated.
Since rx
yields Result<(), _>
it would seem we can't just pass rx
directly. Is there a way we could skip the async move {...}
block and pass a future directly? It would seem we need to import futures::future::FutureExt
to "eat" the Result
? Would that still be preferable to using an async block? Are Axum users likely to have it in their Cargo.toml
already?
cc9cffd
to
4e01db4
Compare
As per mladedav suggestion.
4e01db4
to
2a3a009
Compare
Ping @mladedav Follow-up questions to your comments. |
I'm not sure if this example is more "useful|. I'm using https://lib.rs/crates/vss and it's quite clear how to use it
The current example has this simplicity, not having to introduce Your example is still useful if someone wants to send a signal to axum to shutdown. And I agree that your example enables testing. |
Motivation
The current doc example for
.with_graceful_shutdown()
does not quite show how shutdown/cancellation can be implemented. I think most readers will have a piece of code that looks like this and now wonders how to do shutdown.In this scenario, the main issue is how to handle the fact that
::serve()
blocks.Solution
The easiest and most generally applicable way I can come up with to perform shutdown is to use
tokio::sync::Notify
to signal shutdown, like so:I think this more generally useful pattern since the notify can be passed around and triggered where needed. In contrast, the
select!
strategy implied by the current doc is difficult to use in e.g. integration tests and where the api is a small component of a larger system.