APT-703 Properly Kill Subprocesses With ctrl-c (#81)

Running commands like "rojo serve" and pressing ctrl-c does not kill the
rojo process as intended. Foreman should pass the signal to the tools it
runs. This PR implements the same solution from
[Aftman](LPGhatguy/aftman#13)

Manually tested ctrl-c kills subprocesses on M2 mac and Windows machines
Added 2 scripts to CI to test subprocess are properly killed on linux
and windows machines

Author: afujiwara-roblox

.github/workflows/ci.yml

@@ -70,3 +70,39 @@ jobs:
         foreman --version
         PATH=$PATH:~/.foreman/bin
         ./scripts/end-to-end-tests.sh
+  
+  kill-process-test-unix:
+    strategy:
+      matrix:
+        os: [ubuntu-latest]
+
+    runs-on: ${{ matrix.os }}
+    needs: build
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: kill-process-test-unix
+      shell: bash
+      run: |
+        cargo install --path .
+        foreman --version
+        PATH=$PATH:~/.foreman/bin
+        ./scripts/kill-process-test-unix.sh
+          
+  kill-process-test-windows:
+    strategy:
+      matrix:
+        os: [windows-latest]
+    runs-on: ${{ matrix.os }}
+    needs: build
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: kill-process-test-windows
+      shell: pwsh
+      run: |
+        cargo install --path .
+        foreman --version
+        $env:Path += '%USERPROFILE%/.foreman/bin' 
+        .\scripts\kill-process-test-windows.ps1
+

.github/workflows/clabot.yml

@@ -9,7 +9,7 @@ jobs:
   call-clabot-workflow:
     uses: Roblox/cla-signature-bot/.github/workflows/clabot-workflow.yml@master
     with:
-      whitelist: "LPGhatguy,ZoteTheMighty,cliffchapmanrbx,MagiMaster,MisterUncloaked,amatosov-rbx"
+      whitelist: "LPGhatguy,ZoteTheMighty,cliffchapmanrbx,MagiMaster,MisterUncloaked,amatosov-rbx,afujiwara-roblox"
       use-remote-repo: true
       remote-repo-name: "roblox/cla-bot-store"
     secrets: inherit

Cargo.lock

@@ -34,6 +34,13 @@ toml_edit = "0.14.4"
 urlencoding = "2.1.0"
 zip = "0.5"
 
+[target.'cfg(windows)'.dependencies]
+command-group = "1.0.8"
+
+[target.'cfg(unix)'.dependencies]
+tokio = { version = "1.18.2", features = ["macros", "sync", "process"] }
+signal-hook = "0.3.14"
+
 [dev_dependencies]
 assert_cmd = "2.0.2"
 insta = "1.14"

Cargo.toml

@@ -0,0 +1,55 @@
+#!/bin/bash
+
+write_foreman_toml () {
+    echo "writing foreman.toml"
+    echo "[tools]" > foreman.toml
+    echo "$2 = { $1 = \"$3\", version = \"=$4\" }" >> foreman.toml
+}
+
+create_rojo_files() {
+    echo "writing default.project.json"
+    echo "{
+        \"name\": \"test\",
+        \"tree\": {
+            \"\$path\": \"src\"
+            }
+    }" > default.project.json
+}
+
+setup_rojo() {
+    write_foreman_toml github rojo "rojo-rbx/rojo" "7.3.0"
+    cargo run --release -- install
+    create_rojo_files
+}
+
+kill_process_and_check_delayed() {
+    echo "waiting 5 seconds before killing rojo"
+    sleep 5
+    ps -ef | grep "rojo serve" | grep -v grep | awk '{print $2}' | xargs kill -INT
+    echo "waiting 5 seconds for rojo to be killed"
+    sleep 5
+    check_killed_subprocess
+}
+
+run_rojo_serve_and_kill_process() {
+    setup_rojo
+    (rojo serve default.project.json) & (kill_process_and_check_delayed)
+}
+
+check_killed_subprocess(){
+    echo "checking if process was killed properly"
+    if ps -ef | grep "rojo" | grep -v grep 
+    then 
+        echo "rojo subprocess was not killed properly"
+        rm foreman.toml
+        rm default.project.json
+        exit 1
+    else
+        echo "rojo subprocess was killed properly"
+        rm foreman.toml
+        rm default.project.json
+        exit 0
+    fi
+}
+
+run_rojo_serve_and_kill_process

scripts/kill-process-test-unix.sh

@@ -0,0 +1,55 @@
+function write_foreman_toml($protocol, $tool, $source, $version) {
+    Write-Output "writing foreman.toml"
+    Write-Output "[tools]" | Out-File -FilePath foreman.toml -Encoding utf8
+    Write-Output "$tool = { $protocol = `"$source`", version = `"=$version`" }" | Out-File -FilePath foreman.toml -append -Encoding utf8
+}
+
+function create_rojo_files() {
+    Write-Output "writing default.project.json"
+    Write-Output "{ 
+        `"name`": `"test`", 
+        `"tree`": { 
+            `"`$path`": `"src`"
+        } 
+    }" | Out-File -FilePath default.project.json -Encoding utf8
+}
+
+function setup_rojo() {
+    write_foreman_toml github rojo "rojo-rbx/rojo" "7.3.0"
+    cargo run --release -- install
+    create_rojo_files
+}
+
+function kill_process_and_check_delayed() {
+    Write-Output "waiting 15 seconds before killing rojo"
+    Start-Sleep 15
+    Get-Process | Where-Object { $_.Name -eq "rojo" } | Select-Object -First 1 | Stop-Process
+    Write-Output "waiting 5 seconds to stop rojo"
+    Start-Sleep 5
+    check_killed_subprocess
+}
+
+function run_rojo_serve_and_kill_process() {
+    setup_rojo
+    Start-job -ScriptBlock { rojo serve default.project.json }
+    kill_process_and_check_delayed
+}
+
+function check_killed_subprocess() {
+    Write-Output "Checking if process was killed properly"
+    $rojo = Get-Process -name "rojo-rbx__rojo-7.3.0" -ErrorAction SilentlyContinue
+    if ($rojo) {
+        Write-Output "rojo subprocess was not killed properly"
+        remove-item foreman.toml
+        remove-item default.project.json
+        exit 1
+    }
+    else {
+        Write-Output "rojo subprocess was killed properly"
+        remove-item foreman.toml
+        remove-item default.project.json
+        exit 0
+    }
+}
+
+run_rojo_serve_and_kill_process

scripts/kill-process-test-windows.ps1

@@ -6,10 +6,11 @@ mod config;
 mod error;
 mod fs;
 mod paths;
+mod process;
 mod tool_cache;
 mod tool_provider;
 
-use std::{env, ffi::OsStr, process};
+use std::{env, ffi::OsStr};
 
 use paths::ForemanPaths;
 use structopt::StructOpt;
@@ -67,7 +68,7 @@ impl ToolInvocation {
             let exit_code = tool_cache.run(tool_spec, &version, self.args)?;
 
             if exit_code != 0 {
-                process::exit(exit_code);
+                std::process::exit(exit_code);
             }
 
             Ok(())
@@ -116,7 +117,7 @@ fn main() {
 
 fn exit_with_error(error: ForemanError) -> ! {
     eprintln!("{}", error);
-    process::exit(1);
+    std::process::exit(1);
 }
 
 #[derive(Debug, StructOpt)]

src/main.rs

@@ -0,0 +1,13 @@
+//Orignal source from https://github.com/LPGhatguy/aftman/blob/d3f8d1fac4c89d9163f8f3a0c97fa33b91294fea/src/process/mod.rs
+
+#[cfg(windows)]
+mod windows;
+
+#[cfg(windows)]
+pub use windows::run;
+
+#[cfg(unix)]
+mod unix;
+
+#[cfg(unix)]
+pub use unix::run;

src/process/mod.rs

@@ -0,0 +1,71 @@
+//Original source from https://github.com/LPGhatguy/aftman/blob/d3f8d1fac4c89d9163f8f3a0c97fa33b91294fea/src/process/unix.rs
+
+//! On Unix, we use tokio to spawn processes so that we can listen for signals
+//! and wait for process completion at the same time.
+
+use std::io::{Error, ErrorKind};
+use std::path::Path;
+use std::thread;
+
+use signal_hook::consts::signal::{SIGABRT, SIGINT, SIGQUIT, SIGTERM};
+use signal_hook::iterator::Signals;
+use tokio::process::Command;
+use tokio::sync::oneshot;
+
+pub fn run(exe_path: &Path, args: Vec<String>) -> Result<i32, Error> {
+    let (kill_tx, kill_rx) = oneshot::channel();
+
+    // Spawn a thread dedicated to listening for signals and relaying them to
+    // our async runtime.
+    let (signal_thread, signal_handle) = {
+        let mut signals = Signals::new(&[SIGABRT, SIGINT, SIGQUIT, SIGTERM]).unwrap();
+        let signal_handle = signals.handle();
+
+        let thread = thread::spawn(move || {
+            if let Some(signal) = signals.into_iter().next() {
+                kill_tx.send(signal).ok();
+            }
+        });
+
+        (thread, signal_handle)
+    };
+
+    let runtime = tokio::runtime::Builder::new_current_thread()
+        .enable_io()
+        .build()
+        .map_err(|_| Error::new(ErrorKind::Other, "could not create tokio runtime"))?;
+
+    let _guard = runtime.enter();
+
+    let mut child = Command::new(exe_path).args(args).spawn().map_err(|_| {
+        Error::new(
+            ErrorKind::Other,
+            format!("could not spawn {}", exe_path.display()),
+        )
+    })?;
+
+    let code = runtime.block_on(async move {
+        tokio::select! {
+            // If the child exits cleanly, we can return its exit code directly.
+            // I wish everything were this tidy.
+            status = child.wait() => {
+                let code = status.ok().and_then(|s| s.code()).unwrap_or(1);
+                signal_handle.close();
+                signal_thread.join().unwrap();
+
+                code
+            }
+
+            // If we received a signal while the process was running, murder it
+            // and exit immediately with the correct error code.
+            code = kill_rx => {
+                child.kill().await.ok();
+                signal_handle.close();
+                signal_thread.join().unwrap();
+                std::process::exit(128 + code.unwrap_or(0));
+            }
+        }
+    });
+
+    Ok(code)
+}

src/process/unix.rs

@@ -0,0 +1,26 @@
+//Original source from https://github.com/LPGhatguy/aftman/blob/d3f8d1fac4c89d9163f8f3a0c97fa33b91294fea/src/process/windows.rs
+
+//! On Windows, we use command_group to spawn processes in a job group that will
+//! be automatically cleaned up when this process exits.
+
+use std::io::{Error, ErrorKind};
+use std::path::Path;
+use std::process::Command;
+
+use command_group::CommandGroup;
+
+pub fn run(exe_path: &Path, args: Vec<String>) -> Result<i32, Error> {
+    // On Windows, using a job group here will cause the subprocess to terminate
+    // automatically when Aftman is terminated.
+    let mut child = Command::new(exe_path)
+        .args(args)
+        .group_spawn()
+        .map_err(|_| {
+            Error::new(
+                ErrorKind::Other,
+                format!("Could not spawn {}", exe_path.display()),
+            )
+        })?;
+    let status = child.wait()?;
+    Ok(status.code().unwrap_or(1))
+}