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 |
| 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.
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.
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.
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.
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.
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.