fix(os): attempt to stop children with SIGTERM before SIGKILL (#601)

* fix(os): attempt to stop children with SIGTERM before SIGKILL

* style(fmt): make rustfmt happy

zellij-server/src/os_input_output.rs

@@ -54,8 +54,9 @@ pub(crate) fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) {
 /// Handle some signals for the child process. This will loop until the child
 /// process exits.
 fn handle_command_exit(mut child: Child) {
-    // register the SIGINT signal (TODO handle more signals)
-    let mut signals = signal_hook::iterator::Signals::new(&[SIGINT]).unwrap();
+    let mut should_exit = false;
+    let mut attempts = 3;
+    let mut signals = signal_hook::iterator::Signals::new(&[SIGINT, SIGTERM]).unwrap();
     'handle_exit: loop {
         // test whether the child process has exited
         match child.try_wait() {
@@ -66,17 +67,26 @@ fn handle_command_exit(mut child: Child) {
                 break 'handle_exit;
             }
             Ok(None) => {
-                ::std::thread::sleep(::std::time::Duration::from_millis(100));
+                ::std::thread::sleep(::std::time::Duration::from_millis(10));
             }
             Err(e) => panic!("error attempting to wait: {}", e),
         }
 
-        for signal in signals.pending() {
-            if let SIGINT = signal {
-                child.kill().unwrap();
-                child.wait().unwrap();
-                break 'handle_exit;
+        if !should_exit {
+            for signal in signals.pending() {
+                if signal == SIGINT || signal == SIGTERM {
+                    should_exit = true;
+                }
             }
+        } else if attempts > 0 {
+            // let's try nicely first...
+            attempts -= 1;
+            kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGTERM)).unwrap();
+            continue;
+        } else {
+            // when I say whoa, I mean WHOA!
+            let _ = child.kill();
+            break 'handle_exit;
         }
     }
 }
@@ -90,10 +100,7 @@ fn handle_terminal(cmd: RunCommand, orig_termios: termios::Termios) -> (RawFd, P
             Ok(fork_pty_res) => {
                 let pid_primary = fork_pty_res.master;
                 let pid_secondary = match fork_pty_res.fork_result {
-                    ForkResult::Parent { child } => {
-                        // fcntl(pid_primary, FcntlArg::F_SETFL(OFlag::empty())).expect("could not fcntl");
-                        child
-                    }
+                    ForkResult::Parent { child } => child,
                     ForkResult::Child => {
                         let child = Command::new(cmd.command)
                             .args(&cmd.args)
@@ -205,8 +212,10 @@ pub trait ServerOsApi: Send + Sync {
     fn write_to_tty_stdin(&self, fd: RawFd, buf: &[u8]) -> Result<usize, nix::Error>;
     /// Wait until all output written to the object referred to by `fd` has been transmitted.
     fn tcdrain(&self, fd: RawFd) -> Result<(), nix::Error>;
-    /// Terminate the process with process ID `pid`.
+    /// Terminate the process with process ID `pid`. (SIGTERM)
     fn kill(&self, pid: Pid) -> Result<(), nix::Error>;
+    /// Terminate the process with process ID `pid`. (SIGKILL)
+    fn force_kill(&self, pid: Pid) -> Result<(), nix::Error>;
     /// Returns a [`Box`] pointer to this [`ServerOsApi`] struct.
     fn box_clone(&self) -> Box<dyn ServerOsApi>;
     /// Receives a message on server-side IPC channel
@@ -252,16 +261,14 @@ impl ServerOsApi for ServerOsInputOutput {
         Box::new((*self).clone())
     }
     fn kill(&self, pid: Pid) -> Result<(), nix::Error> {
-        // TODO:
-        // Ideally, we should be using SIGINT rather than SIGKILL here, but there are cases in which
-        // the terminal we're trying to kill hangs on SIGINT and so all the app gets stuck
-        // that's why we're sending SIGKILL here
-        // A better solution would be to send SIGINT here and not wait for it, and then have
-        // a background thread do the waitpid stuff and send SIGKILL if the process is stuck
-        kill(pid, Some(Signal::SIGKILL)).unwrap();
+        kill(pid, Some(Signal::SIGTERM)).unwrap();
         waitpid(pid, None).unwrap();
         Ok(())
     }
+    fn force_kill(&self, pid: Pid) -> Result<(), nix::Error> {
+        let _ = kill(pid, Some(Signal::SIGKILL));
+        Ok(())
+    }
     fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) {
         self.receive_instructions_from_client
             .as_ref()

zellij-server/src/pty.rs

@@ -273,9 +273,20 @@ impl Pty {
             PaneId::Terminal(id) => {
                 let child_pid = self.id_to_child_pid.remove(&id).unwrap();
                 let handle = self.task_handles.remove(&id).unwrap();
-                self.bus.os_input.as_mut().unwrap().kill(child_pid).unwrap();
                 task::block_on(async {
-                    handle.cancel().await;
+                    self.bus.os_input.as_mut().unwrap().kill(child_pid).unwrap();
+                    let timeout = Duration::from_millis(100);
+                    match async_timeout(timeout, handle.cancel()).await {
+                        Ok(_) => {}
+                        _ => {
+                            self.bus
+                                .os_input
+                                .as_mut()
+                                .unwrap()
+                                .force_kill(child_pid)
+                                .unwrap();
+                        }
+                    };
                 });
             }
             PaneId::Plugin(pid) => drop(

zellij-server/src/unit/screen_tests.rs

@@ -44,6 +44,9 @@ impl ServerOsApi for FakeInputOutput {
     fn box_clone(&self) -> Box<dyn ServerOsApi> {
         Box::new((*self).clone())
     }
+    fn force_kill(&self, _pid: Pid) -> Result<(), nix::Error> {
+        unimplemented!()
+    }
     fn kill(&self, _pid: Pid) -> Result<(), nix::Error> {
         unimplemented!()
     }

zellij-server/src/unit/tab_tests.rs

@@ -44,6 +44,9 @@ impl ServerOsApi for FakeInputOutput {
     fn box_clone(&self) -> Box<dyn ServerOsApi> {
         unimplemented!()
     }
+    fn force_kill(&self, _pid: Pid) -> Result<(), nix::Error> {
+        unimplemented!()
+    }
     fn kill(&self, _pid: Pid) -> Result<(), nix::Error> {
         unimplemented!()
     }