Fix signal handling on Unix systems

This removes the the signal handling machinery in tty::unix, and
replaces it with functionality from signal-hook, which should be more
robust. Signals caught by signal-hook wake up the existing I/O event
loop, which then delegates back to the PTY to handle them.

In particular, this allows `SIGCHLD` (i.e. child process exits) to shut
down the terminal promptly, instead of sometimes leaving the window
lingering.

Fixes #915.
Fixes #1276.
Fixes #1313.

As a side effect, this fixes a very rare bug on Linux, where a `read`
from the PTY on the master side would sometimes "fail" with `EIO` if the
child closed the client side at a particular moment. This was subject to
a race condition, and was very difficult to trigger in practice.

7 files changed, 171 insertions, 134 deletions

diff --git a/src/event.rs b/src/event.rs
index cc26bf22..f8044609 100644
--- a/src/event.rs
+++ b/src/event.rs
@@ -176,7 +176,8 @@ impl<'a, N: Notify + 'a> input::ActionContext for ActionContext<'a, N> {
             let proc_prefix = "";
             #[cfg(target_os = "freebsd")]
             let proc_prefix = "/compat/linux";
-            if let Ok(path) = fs::read_link(format!("{}/proc/{}/cwd", proc_prefix, unsafe { tty::PID })) {
+            let link_path = format!("{}/proc/{}/cwd", proc_prefix, tty::child_pid());
+            if let Ok(path) = fs::read_link(link_path) {
                 vec!["--working-directory".into(), path]
             } else {
                 Vec::new()
diff --git a/src/event_loop.rs b/src/event_loop.rs
index 748e2bde..31e803de 100644
--- a/src/event_loop.rs
+++ b/src/event_loop.rs
@@ -34,7 +34,7 @@ pub enum Msg {
 ///
 /// Handles all the pty I/O and runs the pty parser which updates terminal
 /// state.
-pub struct EventLoop<T: tty::EventedReadWrite> {
+pub struct EventLoop<T: tty::EventedPty> {
     poll: mio::Poll,
     pty: T,
     rx: Receiver<Msg>,
@@ -160,12 +160,9 @@ impl Writing {
     }
 }
 
-/// `mio::Token` for the event loop channel
-const CHANNEL: mio::Token = mio::Token(0);
-
 impl<T> EventLoop<T>
     where
-        T: tty::EventedReadWrite + Send + 'static,
+        T: tty::EventedPty + Send + 'static,
 {
     /// Create a new event loop
     pub fn new(
@@ -217,13 +214,13 @@ impl<T> EventLoop<T>
 
     // Returns a `bool` indicating whether or not the event loop should continue running
     #[inline]
-    fn channel_event(&mut self, state: &mut State) -> bool {
+    fn channel_event(&mut self, token: mio::Token, state: &mut State) -> bool {
         if self.drain_recv_channel(state).is_shutdown() {
             return false;
         }
 
         self.poll
-            .reregister(&self.rx, CHANNEL, Ready::readable(), PollOpt::edge() | PollOpt::oneshot())
+            .reregister(&self.rx, token, Ready::readable(), PollOpt::edge() | PollOpt::oneshot())
             .unwrap();
 
         true
@@ -341,17 +338,19 @@ impl<T> EventLoop<T>
             let mut state = state.unwrap_or_else(Default::default);
             let mut buf = [0u8; 0x1000];
 
-            let poll_opts = PollOpt::edge() | PollOpt::oneshot();
+            let mut tokens = (0..).map(Into::into);
 
-            let tokens = [1, 2];
+            let poll_opts = PollOpt::edge() | PollOpt::oneshot();
 
+            let channel_token = tokens.next().unwrap();
             self.poll
-                .register(&self.rx, CHANNEL, Ready::readable(), poll_opts)
+                .register(&self.rx, channel_token, Ready::readable(), poll_opts)
                 .unwrap();
 
             // Register TTY through EventedRW interface
             self.pty
-                .register(&self.poll, &mut tokens.iter(), Ready::readable(), poll_opts).unwrap();
+                .register(&self.poll, &mut tokens, Ready::readable(), poll_opts)
+                .unwrap();
 
             let mut events = Events::with_capacity(1024);
 
@@ -371,41 +370,50 @@ impl<T> EventLoop<T>
 
                 for event in events.iter() {
                     match event.token() {
-                        CHANNEL => if !self.channel_event(&mut state) {
-                            break 'event_loop;
+                        token if token == channel_token => {
+                            if !self.channel_event(channel_token, &mut state) {
+                                break 'event_loop;
+                            }
+                        },
+
+                        #[cfg(unix)]
+                        token if token == self.pty.child_event_token() => {
+                            if let Some(tty::ChildEvent::Exited) = self.pty.next_child_event() {
+                                self.terminal.lock().exit();
+                                self.display.notify();
+                                break 'event_loop;
+                            }
                         },
+
                         token if token == self.pty.read_token() || token == self.pty.write_token() => {
-                            #[cfg(unix)]
-                                {
-                                    if UnixReady::from(event.readiness()).is_hup() {
-                                        break 'event_loop;
-                                    }
+                            #[cfg(unix)] {
+                                if UnixReady::from(event.readiness()).is_hup() {
+                                    // don't try to do I/O on a dead PTY
+                                    continue;
                                 }
+                            }
+
                             if event.readiness().is_readable() {
-                                if let Err(err) = self.pty_read(&mut state, &mut buf, pipe.as_mut())
-                                    {
-                                        error!(
-                                            "[{}:{}] Event loop exiting due to error: {}",
-                                            file!(),
-                                            line!(),
-                                            err
-                                        );
-                                        break 'event_loop;
+                                if let Err(e) = self.pty_read(&mut state, &mut buf, pipe.as_mut()) {
+                                    #[cfg(target_os = "linux")] {
+                                        // On Linux, a `read` on the master side of a PTY can fail
+                                        // with `EIO` if the client side hangs up.  In that case,
+                                        // just loop back round for the inevitable `Exited` event.
+                                        // This sucks, but checking the process is either racy or
+                                        // blocking.
+                                        if e.kind() == ErrorKind::Other {
+                                            continue;
+                                        }
                                     }
 
-                                if crate::tty::process_should_exit() {
+                                    error!("Error reading from PTY in event loop: {}", e);
                                     break 'event_loop;
                                 }
                             }
 
                             if event.readiness().is_writable() {
-                                if let Err(err) = self.pty_write(&mut state) {
-                                    error!(
-                                        "[{}:{}] Event loop exiting due to error: {}",
-                                        file!(),
-                                        line!(),
-                                        err
-                                    );
+                                if let Err(e) = self.pty_write(&mut state) {
+                                    error!("Error writing to PTY in event loop: {}", e);
                                     break 'event_loop;
                                 }
                             }
diff --git a/src/main.rs b/src/main.rs
index 0c7a6b49..4f569cf5 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -236,7 +236,7 @@ fn run(
         }
 
         // Begin shutdown if the flag was raised
-        if terminal_lock.should_exit() {
+        if terminal_lock.should_exit() || tty::process_should_exit() {
             break;
         }
 
diff --git a/src/term/mod.rs b/src/term/mod.rs
index 63b0ca80..fc59ffd6 100644
--- a/src/term/mod.rs
+++ b/src/term/mod.rs
@@ -34,6 +34,8 @@ use crate::url::UrlParser;
 use crate::message_bar::MessageBuffer;
 use crate::term::color::Rgb;
 use crate::term::cell::{LineLength, Cell};
+
+#[cfg(windows)]
 use crate::tty;
 
 pub mod cell;
@@ -1336,7 +1338,7 @@ impl Term {
 
     #[inline]
     pub fn should_exit(&self) -> bool {
-        tty::process_should_exit() || self.should_exit
+        self.should_exit
     }
 }
 
diff --git a/src/tty/mod.rs b/src/tty/mod.rs
index cedb010e..ea2dd0c4 100644
--- a/src/tty/mod.rs
+++ b/src/tty/mod.rs
@@ -40,7 +40,7 @@ pub trait EventedReadWrite {
     fn register(
         &mut self,
         _: &mio::Poll,
-        _: &mut dyn Iterator<Item = &usize>,
+        _: &mut dyn Iterator<Item = mio::Token>,
         _: mio::Ready,
         _: mio::PollOpt,
     ) -> io::Result<()>;
@@ -53,6 +53,29 @@ pub trait EventedReadWrite {
     fn write_token(&self) -> mio::Token;
 }
 
+/// Events concerning TTY child processes
+#[derive(PartialEq)]
+pub enum ChildEvent {
+    /// Indicates the child has exited
+    Exited
+}
+
+/// A pseudoterminal (or PTY)
+///
+/// This is a refinement of EventedReadWrite that also provides a channel through which we can be
+/// notified if the PTY child process does something we care about (other than writing to the TTY).
+/// In particular, this allows for race-free child exit notification on UNIX (cf. `SIGCHLD`).
+pub trait EventedPty : EventedReadWrite {
+    #[cfg(unix)]
+    fn child_event_token(&self) -> mio::Token;
+
+    /// Tries to retrieve an event
+    ///
+    /// Returns `Some(event)` on success, or `None` if there are no events to retrieve.
+    #[cfg(unix)]
+    fn next_child_event(&mut self) -> Option<ChildEvent>;
+}
+
 // Setup environment variables
 pub fn setup_env(config: &Config) {
     // Default to 'alacritty' terminfo if it is available, otherwise
diff --git a/src/tty/unix.rs b/src/tty/unix.rs
index 2a07fc58..d8392dcd 100644
--- a/src/tty/unix.rs
+++ b/src/tty/unix.rs
@@ -15,54 +15,33 @@
 //! tty related functionality
 //!
 
-use crate::tty::EventedReadWrite;
+use crate::tty::{EventedReadWrite, EventedPty, ChildEvent};
 use crate::term::SizeInfo;
 use crate::display::OnResize;
 use crate::config::{Config, Shell};
 use crate::cli::Options;
 use mio;
 
-use libc::{self, c_int, pid_t, winsize, SIGCHLD, TIOCSCTTY, WNOHANG};
+use libc::{self, c_int, pid_t, winsize, TIOCSCTTY};
+use nix::pty::openpty;
+use signal_hook::{self as sighook, iterator::Signals};
 
-use std::os::unix::io::{FromRawFd, RawFd};
+use std::os::unix::{process::CommandExt, io::{FromRawFd, AsRawFd, RawFd}};
 use std::fs::File;
-use std::os::unix::process::CommandExt;
-use std::process::{Command, Stdio};
+use std::process::{Command, Stdio, Child};
 use std::ffi::CStr;
 use std::ptr;
 use mio::unix::EventedFd;
 use std::io;
-use std::os::unix::io::AsRawFd;
-
+use std::sync::atomic::{AtomicUsize, Ordering};
 
 /// Process ID of child process
 ///
 /// Necessary to put this in static storage for `sigchld` to have access
-pub static mut PID: pid_t = 0;
-
-/// Exit flag
-///
-/// Calling exit() in the SIGCHLD handler sometimes causes opengl to deadlock,
-/// and the process hangs. Instead, this flag is set, and its status can be
-/// checked via `process_should_exit`.
-static mut SHOULD_EXIT: bool = false;
-
-extern "C" fn sigchld(_a: c_int) {
-    let mut status: c_int = 0;
-    unsafe {
-        let p = libc::waitpid(PID, &mut status, WNOHANG);
-        if p < 0 {
-            die!("Waiting for pid {} failed: {}\n", PID, errno());
-        }
-
-        if PID == p {
-            SHOULD_EXIT = true;
-        }
-    }
-}
+static PID: AtomicUsize = AtomicUsize::new(0);
 
-pub fn process_should_exit() -> bool {
-    unsafe { SHOULD_EXIT }
+pub fn child_pid() -> pid_t {
+    PID.load(Ordering::Relaxed) as pid_t
 }
 
 /// Get the current value of errno
@@ -71,50 +50,15 @@ fn errno() -> c_int {
 }
 
 /// Get raw fds for master/slave ends of a new pty
-#[cfg(target_os = "linux")]
-fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
-    let mut master: c_int = 0;
-    let mut slave: c_int = 0;
-
-    let win = winsize {
-        ws_row: libc::c_ushort::from(rows),
-        ws_col: libc::c_ushort::from(cols),
-        ws_xpixel: 0,
-        ws_ypixel: 0,
-    };
-
-    let res = unsafe {
-        libc::openpty(&mut master, &mut slave, ptr::null_mut(), ptr::null(), &win)
-    };
-
-    if res < 0 {
-        die!("openpty failed");
-    }
-
-    (master, slave)
-}
-
-#[cfg(any(target_os = "macos",target_os = "freebsd",target_os = "openbsd"))]
-fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
-    let mut master: c_int = 0;
-    let mut slave: c_int = 0;
-
-    let mut win = winsize {
-        ws_row: libc::c_ushort::from(rows),
-        ws_col: libc::c_ushort::from(cols),
-        ws_xpixel: 0,
-        ws_ypixel: 0,
-    };
-
-    let res = unsafe {
-        libc::openpty(&mut master, &mut slave, ptr::null_mut(), ptr::null_mut(), &mut win)
-    };
+fn make_pty(size: winsize) -> (RawFd, RawFd) {
+    let mut win_size = size;
+    win_size.ws_xpixel = 0;
+    win_size.ws_ypixel = 0;
 
-    if res < 0 {
-        die!("openpty failed");
-    }
+    let ends = openpty(Some(&win_size), None)
+        .expect("openpty failed");
 
-    (master, slave)
+    (ends.master, ends.slave)
 }
 
 /// Really only needed on BSD, but should be fine elsewhere
@@ -185,9 +129,12 @@ fn get_pw_entry(buf: &mut [i8; 1024]) -> Passwd<'_> {
 }
 
 pub struct Pty {
+    child: Child,
     pub fd: File,
-    pub raw_fd: RawFd,
     token: mio::Token,
+    signals: Signals,
+    signals_token: mio::Token,
+
 }
 
 impl Pty {
@@ -215,11 +162,11 @@ pub fn new<T: ToWinsize>(
     size: &T,
     window_id: Option<usize>,
 ) -> Pty {
-    let win = size.to_winsize();
+    let win_size = size.to_winsize();
     let mut buf = [0; 1024];
     let pw = get_pw_entry(&mut buf);
 
-    let (master, slave) = openpty(win.ws_row as _, win.ws_col as _);
+    let (master, slave) = make_pty(win_size);
 
     let default_shell = if cfg!(target_os = "macos") {
         let shell_name = pw.shell.rsplit('/').next().unwrap();
@@ -292,15 +239,14 @@ pub fn new<T: ToWinsize>(
         builder.current_dir(dir.as_path());
     }
 
+    // Prepare signal handling before spawning child
+    let signals = Signals::new(&[sighook::SIGCHLD]).expect("error preparing signal handling");
+
     match builder.spawn() {
         Ok(child) => {
-            unsafe {
-                // Set PID for SIGCHLD handler
-                PID = child.id() as _;
+            // Remember child PID so other modules can use it
+            PID.store(child.id() as usize, Ordering::Relaxed);
 
-                // Handle SIGCHLD
-                libc::signal(SIGCHLD, sigchld as _);
-            }
             unsafe {
                 // Maybe this should be done outside of this function so nonblocking
                 // isn't forced upon consumers. Although maybe it should be?
@@ -308,9 +254,11 @@ pub fn new<T: ToWinsize>(
             }
 
             let pty = Pty {
-                fd: unsafe {File::from_raw_fd(master) },
-                raw_fd: master,
-                token: mio::Token::from(0)
+                child,
+                fd: unsafe { File::from_raw_fd(master) },
+                token: mio::Token::from(0),
+                signals,
+                signals_token: mio::Token::from(0),
             };
             pty.resize(size);
             pty
@@ -329,32 +277,53 @@ impl EventedReadWrite for Pty {
     fn register(
         &mut self,
         poll: &mio::Poll,
-        token: &mut dyn Iterator<Item = &usize>,
+        token: &mut dyn Iterator<Item = mio::Token>,
         interest: mio::Ready,
         poll_opts: mio::PollOpt,
     ) -> io::Result<()> {
-        self.token = (*token.next().unwrap()).into();
+        self.token = token.next().unwrap();
         poll.register(
-            &EventedFd(&self.raw_fd),
+            &EventedFd(&self.fd.as_raw_fd()),
             self.token,
             interest,
             poll_opts
+        )?;
+
+        self.signals_token = token.next().unwrap();
+        poll.register(
+            &self.signals,
+            self.signals_token,
+            mio::Ready::readable(),
+            mio::PollOpt::level()
         )
     }
 
     #[inline]
-    fn reregister(&mut self, poll: &mio::Poll, interest: mio::Ready, poll_opts: mio::PollOpt) -> io::Result<()> {
+    fn reregister(
+        &mut self,
+        poll: &mio::Poll,
+        interest: mio::Ready,
+        poll_opts: mio::PollOpt
+    ) -> io::Result<()> {
         poll.reregister(
-            &EventedFd(&self.raw_fd),
+            &EventedFd(&self.fd.as_raw_fd()),
             self.token,
             interest,
             poll_opts
+        )?;
+
+        poll.reregister(
+            &self.signals,
+            self.signals_token,
+            mio::Ready::readable(),
+            mio::PollOpt::level()
         )
     }
 
     #[inline]
     fn deregister(&mut self, poll: &mio::Poll) -> io::Result<()> {
-        poll.deregister(&EventedFd(&self.raw_fd))
+        poll.deregister(&EventedFd(&self.fd.as_raw_fd()))?;
+        poll.deregister(&self.signals)
     }
 
     #[inline]
@@ -378,6 +347,38 @@ impl EventedReadWrite for Pty {
     }
 }
 
+impl EventedPty for Pty {
+    #[inline]
+    fn next_child_event(&mut self) -> Option<ChildEvent> {
+        self.signals
+            .pending()
+            .next()
+            .and_then(|signal| {
+                if signal != sighook::SIGCHLD {
+                    return None;
+                }
+
+                match self.child.try_wait() {
+                    Err(e) => {
+                        error!("Error checking child process termination: {}", e);
+                        None
+                    },
+                    Ok(None) => None,
+                    Ok(_)    => Some(ChildEvent::Exited),
+                }
+            })
+    }
+
+    #[inline]
+    fn child_event_token(&self) -> mio::Token {
+        self.signals_token
+    }
+}
+
+pub fn process_should_exit() -> bool {
+    false
+}
+
 /// Types that can produce a `libc::winsize`
 pub trait ToWinsize {
     /// Get a `libc::winsize`
diff --git a/src/tty/windows/mod.rs b/src/tty/windows/mod.rs
index 5e0aee84..2f5caa93 100644
--- a/src/tty/windows/mod.rs
+++ b/src/tty/windows/mod.rs
@@ -28,7 +28,7 @@ use crate::cli::Options;
 use crate::config::Config;
 use crate::display::OnResize;
 use crate::term::SizeInfo;
-use crate::tty::EventedReadWrite;
+use crate::tty::{EventedReadWrite, EventedPty};
 
 mod conpty;
 mod winpty;
@@ -232,12 +232,12 @@ impl<'a> EventedReadWrite for Pty<'a> {
     fn register(
         &mut self,
         poll: &mio::Poll,
-        token: &mut dyn Iterator<Item = &usize>,
+        token: &mut dyn Iterator<Item = mio::Token>,
         interest: mio::Ready,
         poll_opts: mio::PollOpt,
     ) -> io::Result<()> {
-        self.read_token = (*token.next().unwrap()).into();
-        self.write_token = (*token.next().unwrap()).into();
+        self.read_token = token.next().unwrap();
+        self.write_token = token.next().unwrap();
 
         if interest.is_readable() {
             poll.register(
@@ -339,3 +339,5 @@ impl<'a> EventedReadWrite for Pty<'a> {
         self.write_token
     }
 }
+
+impl<'a> EventedPty for Pty<'a> { }