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

feat: Kill all child processes when shell received CTRL-C #125

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ anyhow = "1.0.75"
futures = { version = "0.3.29", optional = true }
glob = { version = "0.3.1", optional = true }
path-dedot = { version = "3.1.1", optional = true }
tokio = { version = "1", features = ["fs", "io-std", "io-util", "macros", "process", "rt-multi-thread", "sync", "time"], optional = true }
tokio = { version = "1", features = ["fs", "io-std", "io-util", "macros", "process", "rt-multi-thread", "sync", "time", "signal"], optional = true }
tokio-util = { version = "0.7.10", optional = true }
os_pipe = { version = "1.1.4", optional = true }
serde = { version = "1", features = ["derive"], optional = true }
Expand Down
45 changes: 29 additions & 16 deletions src/shell/commands/executable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ use crate::FutureExecuteResult;
use crate::ShellCommand;
use crate::ShellCommandContext;
use futures::FutureExt;
use std::sync::Arc;
use tokio::sync::Mutex;

/// Command that resolves the command name and
/// executes it in a separate process.
Expand Down Expand Up @@ -41,8 +43,8 @@ impl ShellCommand for ExecutableCommand {
.stderr(stderr.clone().into_stdio())
.spawn();

let mut child = match child {
Ok(child) => child,
let child = match child {
Ok(child) => Arc::new(Mutex::new(child)),
Err(err) => {
let _ = stderr.write_line(&format!(
"Error launching '{}': {}",
Expand All @@ -55,22 +57,33 @@ impl ShellCommand for ExecutableCommand {
// avoid deadlock since this is holding onto the pipes
drop(sub_command);

tokio::select! {
result = child.wait() => match result {
Ok(status) => ExecuteResult::Continue(
status.code().unwrap_or(1),
Vec::new(),
Vec::new(),
),
Err(err) => {
let _ = stderr.write_line(&format!("{}", err));
ExecuteResult::Continue(1, Vec::new(), Vec::new())
}
},
_ = context.state.token().cancelled() => {
let child_clone = Arc::clone(&child);

tokio::spawn(async move {
tokio::signal::ctrl_c().await.unwrap();
let mut child = child_clone.lock().await;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how this works because acquiring the lock in one place will block the other.

Instead of spawning a task, I feel like it should work to just add this to the select below? Something like:

      tokio::select! {
          result = child.wait() => /* ... */,
          _ = tokio::signal::ctrl_c() => {
              let _ = child.kill().await;
              // ...
          }
          _ = context.state.token().cancelled() => {
              let _ = child.kill().await;
              ExecuteResult::for_cancellation()
          }
      }

By the way, in the future when opening a PR, make sure to do it from a branch instead of the main branch. There's a couple reasons to do this:

  1. It makes it easy to merge the latest main into your fork because your main branch is clean.
  2. It allows maintaininers to push to your branch. Right now I get:
    To https://github.com/mo/deno_task_shell.git
     ! [remote rejected] mo/main -> mo/main (permission denied)
    error: failed to push some refs to 'https://github.com/mo/deno_task_shell.git'
    

if let Ok(None) = child.try_wait() {
let _ = child.kill().await;
ExecuteResult::for_cancellation()
}
});

let mut child_locked = child.lock().await;
tokio::select! {
result = child_locked.wait() => match result {
Ok(status) => ExecuteResult::Continue(
status.code().unwrap_or(1),
Vec::new(),
Vec::new(),
),
Err(err) => {
let _ = stderr.write_line(&format!("{}", err));
ExecuteResult::Continue(1, Vec::new(), Vec::new())
}
},
_ = context.state.token().cancelled() => {
let _ = child_locked.kill().await;
ExecuteResult::for_cancellation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of wonder if we should just add a single listener in Deno for ctrl+c then cancel the token on ShellState (we'd have to manually set it up to use https://docs.rs/deno_task_shell/0.18.1/deno_task_shell/fn.execute_with_pipes.html though).

}
}
}
.boxed_local()
Expand Down