-
Notifications
You must be signed in to change notification settings - Fork 50
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
On worker stop #1195
base: main
Are you sure you want to change the base?
On worker stop #1195
Conversation
The on stop callback looks ok. Or alternatively the on stop callback can be just an The Do you mind split this PR and focus on the work stop callback/async future for now? The |
@@ -60,6 +61,7 @@ impl Server { | |||
let is_graceful_shutdown = Arc::new(AtomicBool::new(false)); | |||
|
|||
let on_start_fut = on_worker_start(); | |||
let on_stop_fut = on_worker_stop(); |
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 closure executes eagerly on worker start meaning
|| {
// happens when worker start.
async {
// happens when worker stop.
}
}
This can be a potential footgun.
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.
Yup it shot me as well. Should we make this more similar to tokio's on_thread_start/on_thread_stop callbacks that just fake a non-async function?
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.
My opinion is we make it accepting a future rather than a closure. Reason being worker itself runs in an async context already so there is no need to do async { closure().await }
and we can just do async { future.await }
.
on_worker_start
and tokio's case are different because these APIs have access to non async context (or partially so )
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.
Unfortunately I can't think of how that would work since the future would need to be awaited multiple times, once per worker task thread and that's not a power that future's have. I think that's why you used a generator pattern where the callback vends a new future each time and why Tokio doesn't have this problem since a normal Fn can be invoked multiple times.
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.
If you don't mind waiting on this PR for a bit we can switch to async closure when Rust 1.85 release later this month. xitca as a whole would switch to Rust 1.85 and edition 2024 at the same time. After which we can use on_worker_stop(async || {})
so there is no accidental eager execution on the closure.
69bdec3
to
8bc7bb7
Compare
If setting up TLS variables in on_worker_start, they need to be torn down in on_worker_stop or you might end up accidentally accessing destroyed TLS variables.
8bc7bb7
to
2af9893
Compare
Without an on_worker_stop, my app crashes during shutdown because things end up trying to access TLS variables that were destroyed already. This is similar parity to the Tokio runtime on_thread_start/on_thread_stop for the same reason.
This resolves part of #1187.