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

On worker stop #1195

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

On worker stop #1195

wants to merge 1 commit into from

Conversation

vlovich
Copy link

@vlovich vlovich commented Feb 1, 2025

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.

@fakeshadow
Copy link
Collaborator

fakeshadow commented Feb 1, 2025

The on stop callback looks ok. Or alternatively the on stop callback can be just an async {} without the closure.

The AsListener trait is planned to be reworked before exporting and you can reference #1186 for some context. The issue is the trait is hacky and was not considered a problem being only used inside the crate. For a public trait it's not in good shape and need some rework before it can be exported.

Do you mind split this PR and focus on the work stop callback/async future for now? The AsListener trait problem will be fixed separately and hopefully it would be ergonomic without your further effort on it.

@@ -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();
Copy link
Collaborator

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.

Copy link
Author

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?

Copy link
Collaborator

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 )

Copy link
Author

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.

Copy link
Collaborator

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants