Remove async code from the Drop handler and make cancellation more gentle for uv#90
Remove async code from the Drop handler and make cancellation more gentle for uv#90
uv#90Conversation
bradhe
changed the title
uv
There was a problem hiding this comment.
Pull Request Overview
This PR removes async code from the Drop handler for LocalApp and implements more graceful process termination for uv child processes. The changes address issues where terminate was never called due to async constraints and improves child process cleanup by using SIGTERM before SIGKILL.
- Remove async code from Drop implementation and simplify termination logic
- Add process group configuration to uv command spawning for better process management
- Implement graceful child process termination with SIGTERM followed by SIGKILL timeout
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/tower-runtime/src/local.rs | Refactored Drop handler to be non-async, simplified terminator field, and added graceful process killing |
| crates/tower-uv/src/lib.rs | Added process_group(0) to all uv command spawns for better process group management |
| crates/tower-runtime/Cargo.toml | Added nix dependency for Unix signal handling |
| Cargo.toml | Added nix workspace dependency with signal features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crates/tower-runtime/src/local.rs
Outdated
| nix::sys::signal::Signal::SIGTERM | ||
| ).ok(); | ||
|
|
||
| // If it doesn't die after 5 seconds then we'll forcefullt kill it. |
There was a problem hiding this comment.
There's a typo in the comment: 'forcefullt' should be 'forcefully'.
| // If it doesn't die after 5 seconds then we'll forcefullt kill it. | |
| // If it doesn't die after 5 seconds then we'll forcefully kill it. |
crates/tower-runtime/src/local.rs
Outdated
|
|
||
| #[cfg(unix)] | ||
| async fn kill_child_process(ctx: &tower_telemetry::Context, mut child: Child) { | ||
| let pid = child.id().unwrap(); |
There was a problem hiding this comment.
Using unwrap() on child.id() could panic if the process ID is unavailable. Consider handling this error case gracefully or adding a comment explaining why panic is acceptable here.
| let pid = child.id().unwrap(); | |
| let pid = match child.id() { | |
| Some(pid) => pid, | |
| None => { | |
| debug!(ctx: &ctx, "Failed to get child process ID; process may have already exited."); | |
| return; | |
| } | |
| }; |
crates/tower-runtime/src/local.rs
Outdated
| nix::sys::signal::killpg( | ||
| nix::unistd::Pid::from_raw(pid as i32), | ||
| nix::sys::signal::Signal::SIGTERM | ||
| ).ok(); |
There was a problem hiding this comment.
[nitpick] The PID conversion nix::unistd::Pid::from_raw(pid as i32) is duplicated. Consider extracting it to a variable to improve readability and reduce duplication.
crates/tower-runtime/src/local.rs
Outdated
| nix::sys::signal::killpg( | ||
| nix::unistd::Pid::from_raw(pid as i32), | ||
| nix::sys::signal::Signal::SIGKILL | ||
| ).ok(); |
There was a problem hiding this comment.
[nitpick] The PID conversion nix::unistd::Pid::from_raw(pid as i32) is duplicated. Consider extracting it to a variable to improve readability and reduce duplication.
| if timeout.is_err() { | ||
| killpg( | ||
| pid, | ||
| Signal::SIGKILL |
There was a problem hiding this comment.
maybe a bit old school but i reckon a SIGHUP before SIGKILL might be an idea
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| async fn kill_child_process(ctx: &tower_telemetry::Context, mut child: Child) { |
There was a problem hiding this comment.
Ok so in windows will default back to whatever child.kill() tries to do
There was a problem hiding this comment.
Yes, apparently (untested by me) it's not an issue on Windows.
Co-authored-by: Konstantinos St <kons.ste@gmail.com>
* Remove async code from the Drop handler and make cancellation more gentle for `uv` (#90) * Remove async code from the Drop handler * feat: More graceful process shutdown to be gentle to uv * chore: Refactor kill_child_process based on @copilot feedback * chore: Only use process_group on unix systems * chore: Only import the nix::* content in Unix land * Update crates/tower-runtime/src/local.rs Co-authored-by: Konstantinos St <kons.ste@gmail.com> --------- Co-authored-by: Konstantinos St <kons.ste@gmail.com> * Bump version to v0.3.28 * chore: Avoid unneeded import on Windows --------- Co-authored-by: Konstantinos St <kons.ste@gmail.com>
Since the code was async,
terminatewas never actually called. In order to avoid all this crazy, we just push all the async code out (as much as possible) to make things simpler. We don't explicitly need to join the execution handler after the child is terminated...it just seems like the right thing to do to ensure the Drop implementation waits for the whole thing to be dead.On top of that, I figured out that
uvwill leave child processes floating about if you useSIGKILL-- so we need to work around it a bit withSIGTERMbeing sent first.