Address comments

- Remove `unwrap()` calls
- Implement `exit_code_from_signal` for windows platform.rs
- Refactored use statements
- Cleaned up commented code
- Increased sleep time in between polling to 50ms
- Simplified Signal registering logic

src/config.rs

@@ -683,7 +683,7 @@ impl Config {
 
   pub(crate) fn run(self, loader: &Loader) -> Result<(), Error> {
     if let Err(error) = SignalHandler::install(self.verbosity) {
-      warn!("Failed to set CTRL-C handler: {error}");
+      warn!("Failed to set signal handler: {error}");
     }
 
     self.subcommand.execute(&self, loader)

src/platform.rs

@@ -38,6 +38,7 @@ impl PlatformInterface for Platform {
     exit_status.signal()
   }
 
+  /// See [Exit Codes With Special Meanings](https://tldp.org/LDP/abs/html/exitcodes.html)
   fn exit_code_from_signal(signal: i32) -> i32 {
     signal + 128
   }
@@ -101,6 +102,10 @@ impl PlatformInterface for Platform {
     None
   }
 
+  fn exit_code_from_signal(signal: i32) -> i32 {
+    signal
+  }
+
   fn convert_native_path(working_directory: &Path, path: &Path) -> Result<String, String> {
     // Translate path from windows style to unix style
     let mut cygpath = Command::new("cygpath");

src/platform_interface.rs

@@ -16,9 +16,7 @@ pub(crate) trait PlatformInterface {
   /// signal
   fn signal_from_exit_status(exit_status: ExitStatus) -> Option<i32>;
 
-  /// Convert a signal into an exit code, for the case where a process was
-  /// terminated by a signal
-  /// See https://tldp.org/LDP/abs/html/exitcodes.html
+  /// Convert a signal number into an exit code
   fn exit_code_from_signal(signal: i32) -> i32;
 
   /// Translate a path from a "native" path to a path the interpreter expects

src/run.rs

@@ -44,8 +44,7 @@ pub fn run() -> Result<(), i32> {
           Error::CommandStatus { status, .. } => match status.code() {
             Some(code) => code,
             None => Platform::signal_from_exit_status(status)
-              .map(Platform::exit_code_from_signal)
-              .unwrap_or(EXIT_FAILURE),
+              .map_or(EXIT_FAILURE, Platform::exit_code_from_signal),
           },
           _ => EXIT_FAILURE,
         },

src/signal_handler.rs

@@ -1,63 +1,47 @@
-use super::*;
-
-use command_group::{CommandGroup, GroupChild};
-use signal_hook::consts::signal::*;
-use signal_hook::consts::TERM_SIGNALS;
-use signal_hook::flag;
-use signal_hook::iterator::exfiltrator::WithOrigin;
-use signal_hook::iterator::SignalsInfo;
-use std::io::Read;
-use std::process::Command;
-use std::sync::{atomic::AtomicBool, Arc};
+use {
+  super::*,
+  command_group::{CommandGroup, GroupChild},
+  signal_hook::{
+    consts::{signal::*, TERM_SIGNALS},
+    iterator::{exfiltrator::WithOrigin, SignalsInfo},
+  },
+  std::{io::Read, process::Command},
+};
 
 pub(crate) struct SignalHandler {
   verbosity: Verbosity,
-  signals: Option<SignalsInfo<WithOrigin>>,
+  signals_info: Option<SignalsInfo<WithOrigin>>,
 }
 
 impl SignalHandler {
   pub(crate) fn install(verbosity: Verbosity) -> Result<(), std::io::Error> {
+    const MISC_SIGNALS: &[i32] = &[SIGHUP];
+
     let mut instance = Self::instance();
     instance.verbosity = verbosity;
 
-    let signal_flag = Arc::new(AtomicBool::new(false));
-
-    const MISC_SIGNALS: &[i32] = &[
-      #[cfg(not(windows))]
-      SIGHUP,
-      // SIGABRT,
-      // SIGPIPE,
-      // SIGALRM,
-    ];
-
-    let mut sigs = MISC_SIGNALS.to_vec();
-    sigs.extend(TERM_SIGNALS);
+    let mut signals = MISC_SIGNALS.to_vec();
+    signals.extend(TERM_SIGNALS);
 
-    let signals = SignalsInfo::<WithOrigin>::new(&sigs)?;
-    instance.signals = Some(signals);
+    let signals_info = SignalsInfo::<WithOrigin>::new(&signals)?;
+    instance.signals_info = Some(signals_info);
 
-    for sig in TERM_SIGNALS {
-      // NOTE This could be set before the register handler, in order to terminate in a double ctrl-c
-      // flag::register_conditional_shutdown(*sig, 1, Arc::clone(&signal_flag))?;
-      flag::register(*sig, Arc::clone(&signal_flag))?;
-    }
-
-    for sig in MISC_SIGNALS {
-      flag::register(*sig, Arc::clone(&signal_flag))?;
-    }
     Ok(())
   }
 
-  pub(crate) fn instance() -> MutexGuard<'static, Self> {
-    static INSTANCE: Mutex<SignalHandler> = Mutex::new(SignalHandler::new());
+  fn instance() -> MutexGuard<'static, Self> {
+    static INSTANCE: Mutex<SignalHandler> = Mutex::new(SignalHandler {
+      verbosity: Verbosity::default(),
+      signals_info: None,
+    });
 
     match INSTANCE.lock() {
       Ok(guard) => guard,
       Err(poison_error) => {
         eprintln!(
           "{}",
           Error::Internal {
-            message: format!("interrupt handler mutex poisoned: {poison_error}"),
+            message: format!("signal handler mutex poisoned: {poison_error}"),
           }
           .color_display(Color::auto().stderr())
         );
@@ -66,27 +50,32 @@ impl SignalHandler {
     }
   }
 
-  const fn new() -> Self {
-    Self {
-      verbosity: Verbosity::default(),
-      signals: None,
-    }
-  }
-
   fn wait_child(mut child: GroupChild) -> std::io::Result<(GroupChild, ExitStatus)> {
     let mut instance = Self::instance();
     loop {
-      if let Some(signals) = instance.signals.as_mut() {
-        for signal_info in signals.pending() {
-          unsafe {
-            // Send signal to the child process group, thus the negative pid
-            libc::kill(-(child.id() as i32), signal_info.signal);
+      if let Some(signals_info) = instance.signals_info.as_mut() {
+        for info in signals_info.pending() {
+          let child_pid = child.id();
+          if let Ok(pid) = i32::try_from(child_pid) {
+            unsafe {
+              // Forward signal to the process group using negative pid
+              libc::kill(-pid, info.signal);
+            }
+          } else {
+            eprintln!(
+              "{}",
+              Error::Internal {
+                message: format!("Could not convert child pid to i32: {child_pid}"),
+              }
+              .color_display(Color::auto().stderr())
+            );
+            process::exit(EXIT_FAILURE);
           }
         }
       }
       match child.try_wait() {
         Ok(None) => {
-          std::thread::sleep(std::time::Duration::from_millis(10));
+          std::thread::sleep(std::time::Duration::from_millis(50));
           // If the child process is still running, continue polling
           continue;
         }
@@ -105,7 +94,7 @@ impl SignalHandler {
     let child = command
       .stdout(std::process::Stdio::piped())
       .group_spawn()
-      .unwrap();
+      .map_err(OutputError::Io)?;
     match Self::wait_child(child) {
       Ok((mut child, status)) => {
         if let Some(code) = status.code() {
@@ -122,7 +111,7 @@ impl SignalHandler {
         match child.inner().stdout.as_mut() {
           Some(stdout) => {
             let mut buffer = Vec::new();
-            stdout.read_to_end(&mut buffer).unwrap();
+            stdout.read_to_end(&mut buffer).map_err(OutputError::Io)?;
             match str::from_utf8(buffer.as_slice()) {
               Err(error) => Err(OutputError::Utf8(error)),
               Ok(utf8) => Ok(
@@ -145,7 +134,7 @@ impl SignalHandler {
   }
 
   pub(crate) fn guard(mut command: Command) -> Result<std::process::ExitStatus, std::io::Error> {
-    let child = command.group_spawn().unwrap();
+    let child = command.group_spawn()?;
     match Self::wait_child(child) {
       Ok((_, status)) => Ok(status),
       Err(error) => Err(error),