Fix WinPTY freeze on termination

Fixes #2889.

8 files changed, 171 insertions, 54 deletions

diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs
index 1fc1ee42..2c171e23 100644
--- a/alacritty/src/event.rs
+++ b/alacritty/src/event.rs
@@ -30,6 +30,7 @@ use alacritty_terminal::selection::Selection;
 use alacritty_terminal::sync::FairMutex;
 use alacritty_terminal::term::cell::Cell;
 use alacritty_terminal::term::{SizeInfo, Term};
+#[cfg(not(windows))]
 use alacritty_terminal::tty;
 use alacritty_terminal::util::{limit, start_daemon};
 
@@ -351,14 +352,14 @@ impl<N: Notify> Processor<N> {
                 info!("glutin event: {:?}", event);
             }
 
-            match (&event, tty::process_should_exit()) {
+            match &event {
                 // Check for shutdown
-                (GlutinEvent::UserEvent(Event::Exit), _) | (_, true) => {
+                GlutinEvent::UserEvent(Event::Exit) => {
                     *control_flow = ControlFlow::Exit;
                     return;
                 },
                 // Process events
-                (GlutinEvent::EventsCleared, _) => {
+                GlutinEvent::EventsCleared => {
                     *control_flow = ControlFlow::Wait;
 
                     if event_queue.is_empty() {
diff --git a/alacritty_terminal/src/event_loop.rs b/alacritty_terminal/src/event_loop.rs
index 3e762840..00f77c9f 100644
--- a/alacritty_terminal/src/event_loop.rs
+++ b/alacritty_terminal/src/event_loop.rs
@@ -250,8 +250,10 @@ where
             }
         }
 
-        // Queue terminal redraw
-        self.event_proxy.send_event(Event::Wakeup);
+        if processed > 0 {
+            // Queue terminal redraw
+            self.event_proxy.send_event(Event::Wakeup);
+        }
 
         Ok(())
     }
@@ -327,7 +329,6 @@ where
                             }
                         },
 
-                        #[cfg(unix)]
                         token if token == self.pty.child_event_token() => {
                             if let Some(tty::ChildEvent::Exited) = self.pty.next_child_event() {
                                 if !self.hold {
diff --git a/alacritty_terminal/src/tty/mod.rs b/alacritty_terminal/src/tty/mod.rs
index 7150a42d..40d019f5 100644
--- a/alacritty_terminal/src/tty/mod.rs
+++ b/alacritty_terminal/src/tty/mod.rs
@@ -54,7 +54,7 @@ pub trait EventedReadWrite {
 }
 
 /// Events concerning TTY child processes
-#[derive(PartialEq)]
+#[derive(Debug, PartialEq)]
 pub enum ChildEvent {
     /// Indicates the child has exited
     Exited,
@@ -66,13 +66,11 @@ pub enum ChildEvent {
 /// 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>;
 }
 
diff --git a/alacritty_terminal/src/tty/unix.rs b/alacritty_terminal/src/tty/unix.rs
index b820d754..01ee1b59 100644
--- a/alacritty_terminal/src/tty/unix.rs
+++ b/alacritty_terminal/src/tty/unix.rs
@@ -333,10 +333,6 @@ impl EventedPty for Pty {
     }
 }
 
-pub fn process_should_exit() -> bool {
-    false
-}
-
 /// Types that can produce a `libc::winsize`
 pub trait ToWinsize {
     /// Get a `libc::winsize`
diff --git a/alacritty_terminal/src/tty/windows/child.rs b/alacritty_terminal/src/tty/windows/child.rs
new file mode 100644
index 00000000..447b7fbf
--- /dev/null
+++ b/alacritty_terminal/src/tty/windows/child.rs
@@ -0,0 +1,115 @@
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::ffi::c_void;
+use std::io::Error;
+use std::sync::atomic::{AtomicPtr, Ordering};
+
+use mio_extras::channel::{channel, Receiver, Sender};
+
+use winapi::shared::ntdef::{BOOLEAN, HANDLE, PVOID};
+use winapi::um::winbase::{RegisterWaitForSingleObject, UnregisterWait, INFINITE};
+use winapi::um::winnt::{WT_EXECUTEINWAITTHREAD, WT_EXECUTEONLYONCE};
+
+use crate::tty::ChildEvent;
+
+/// WinAPI callback to run when child process exits.
+extern "system" fn child_exit_callback(ctx: PVOID, timed_out: BOOLEAN) {
+    if timed_out != 0 {
+        return;
+    }
+
+    let event_tx: Box<_> = unsafe { Box::from_raw(ctx as *mut Sender<ChildEvent>) };
+    let _ = event_tx.send(ChildEvent::Exited);
+}
+
+pub struct ChildExitWatcher {
+    wait_handle: AtomicPtr<c_void>,
+    event_rx: Receiver<ChildEvent>,
+}
+
+impl ChildExitWatcher {
+    pub fn new(child_handle: HANDLE) -> Result<ChildExitWatcher, Error> {
+        let (event_tx, event_rx) = channel::<ChildEvent>();
+
+        let mut wait_handle: HANDLE = 0 as HANDLE;
+        let sender_ref = Box::new(event_tx);
+
+        let success = unsafe {
+            RegisterWaitForSingleObject(
+                &mut wait_handle,
+                child_handle,
+                Some(child_exit_callback),
+                Box::into_raw(sender_ref) as PVOID,
+                INFINITE,
+                WT_EXECUTEINWAITTHREAD | WT_EXECUTEONLYONCE,
+            )
+        };
+
+        if success == 0 {
+            Err(Error::last_os_error())
+        } else {
+            Ok(ChildExitWatcher { wait_handle: AtomicPtr::from(wait_handle), event_rx })
+        }
+    }
+
+    pub fn event_rx(&self) -> &Receiver<ChildEvent> {
+        &self.event_rx
+    }
+}
+
+impl Drop for ChildExitWatcher {
+    fn drop(&mut self) {
+        unsafe {
+            UnregisterWait(self.wait_handle.load(Ordering::Relaxed));
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use std::os::windows::io::AsRawHandle;
+    use std::process::Command;
+    use std::time::Duration;
+
+    use mio::{Events, Poll, PollOpt, Ready, Token};
+
+    use super::*;
+
+    #[test]
+    pub fn event_is_emitted_when_child_exits() {
+        const WAIT_TIMEOUT: Duration = Duration::from_millis(200);
+
+        let mut child = Command::new("cmd.exe").spawn().unwrap();
+        let child_exit_watcher = ChildExitWatcher::new(child.as_raw_handle()).unwrap();
+
+        let mut events = Events::with_capacity(1);
+        let poll = Poll::new().unwrap();
+        let child_events_token = Token::from(0usize);
+
+        poll.register(
+            child_exit_watcher.event_rx(),
+            child_events_token,
+            Ready::readable(),
+            PollOpt::oneshot(),
+        )
+        .unwrap();
+
+        child.kill().unwrap();
+
+        // Poll for the event or fail with timeout if nothing has been sent
+        poll.poll(&mut events, Some(WAIT_TIMEOUT)).unwrap();
+        assert_eq!(events.iter().next().unwrap().token(), child_events_token);
+        // Verify that at least one `ChildEvent::Exited` was received
+        assert_eq!(child_exit_watcher.event_rx().try_recv(), Ok(ChildEvent::Exited));
+    }
+}
diff --git a/alacritty_terminal/src/tty/windows/conpty.rs b/alacritty_terminal/src/tty/windows/conpty.rs
index 185acfc2..f34ad922 100644
--- a/alacritty_terminal/src/tty/windows/conpty.rs
+++ b/alacritty_terminal/src/tty/windows/conpty.rs
@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use super::{Pty, HANDLE};
-
 use std::i16;
 use std::io::Error;
 use std::mem;
@@ -22,7 +20,6 @@ use std::ptr;
 use std::sync::Arc;
 
 use dunce::canonicalize;
-use log::info;
 use mio_anonymous_pipes::{EventedAnonRead, EventedAnonWrite};
 use miow;
 use widestring::U16CString;
@@ -41,6 +38,8 @@ use winapi::um::wincontypes::{COORD, HPCON};
 use crate::config::{Config, Shell};
 use crate::event::OnResize;
 use crate::term::SizeInfo;
+use crate::tty::windows::child::ChildExitWatcher;
+use crate::tty::windows::Pty;
 
 /// Dynamically-loaded Pseudoconsole API from kernel32.dll
 ///
@@ -241,14 +240,10 @@ pub fn new<'a, C>(
         }
     }
 
-    // Store handle to console
-    unsafe {
-        HANDLE = proc_info.hProcess;
-    }
-
     let conin = EventedAnonWrite::new(conin);
     let conout = EventedAnonRead::new(conout);
 
+    let child_watcher = ChildExitWatcher::new(proc_info.hProcess).unwrap();
     let agent = Conpty { handle: pty_handle, api };
 
     Some(Pty {
@@ -257,6 +252,8 @@ pub fn new<'a, C>(
         conin: super::EventedWritablePipe::Anonymous(conin),
         read_token: 0.into(),
         write_token: 0.into(),
+        child_event_token: 0.into(),
+        child_watcher,
     })
 }
 
diff --git a/alacritty_terminal/src/tty/windows/mod.rs b/alacritty_terminal/src/tty/windows/mod.rs
index 922dde26..436b5edf 100644
--- a/alacritty_terminal/src/tty/windows/mod.rs
+++ b/alacritty_terminal/src/tty/windows/mod.rs
@@ -13,49 +13,27 @@
 // limitations under the License.
 
 use std::io::{self, Read, Write};
-use std::os::raw::c_void;
 use std::sync::atomic::{AtomicBool, Ordering};
+use std::sync::mpsc::TryRecvError;
 
 use mio::{self, Evented, Poll, PollOpt, Ready, Token};
 use mio_anonymous_pipes::{EventedAnonRead, EventedAnonWrite};
 use mio_named_pipes::NamedPipe;
 
 use log::info;
-use winapi::shared::winerror::WAIT_TIMEOUT;
-use winapi::um::synchapi::WaitForSingleObject;
-use winapi::um::winbase::WAIT_OBJECT_0;
 
 use crate::config::Config;
 use crate::event::OnResize;
 use crate::term::SizeInfo;
-use crate::tty::{EventedPty, EventedReadWrite};
+use crate::tty::windows::child::ChildExitWatcher;
+use crate::tty::{ChildEvent, EventedPty, EventedReadWrite};
 
+mod child;
 mod conpty;
 mod winpty;
 
-/// Handle to the winpty agent or conpty process. Required so we know when it closes.
-static mut HANDLE: *mut c_void = 0usize as *mut c_void;
 static IS_CONPTY: AtomicBool = AtomicBool::new(false);
 
-pub fn process_should_exit() -> bool {
-    unsafe {
-        match WaitForSingleObject(HANDLE, 0) {
-            // Process has exited
-            WAIT_OBJECT_0 => {
-                info!("wait_object_0");
-                true
-            },
-            // Reached timeout of 0, process has not exited
-            WAIT_TIMEOUT => false,
-            // Error checking process, winpty gave us a bad agent handle?
-            _ => {
-                info!("Bad exit: {}", ::std::io::Error::last_os_error());
-                true
-            },
-        }
-    }
-}
-
 pub fn is_conpty() -> bool {
     IS_CONPTY.load(Ordering::Relaxed)
 }
@@ -76,6 +54,8 @@ pub struct Pty<'a> {
     conin: EventedWritablePipe,
     read_token: mio::Token,
     write_token: mio::Token,
+    child_event_token: mio::Token,
+    child_watcher: ChildExitWatcher,
 }
 
 impl<'a> Pty<'a> {
@@ -244,6 +224,15 @@ impl<'a> EventedReadWrite for Pty<'a> {
         } else {
             poll.register(&self.conin, self.write_token, mio::Ready::empty(), poll_opts)?
         }
+
+        self.child_event_token = token.next().unwrap();
+        poll.register(
+            self.child_watcher.event_rx(),
+            self.child_event_token,
+            mio::Ready::readable(),
+            poll_opts,
+        )?;
+
         Ok(())
     }
 
@@ -264,6 +253,14 @@ impl<'a> EventedReadWrite for Pty<'a> {
         } else {
             poll.reregister(&self.conin, self.write_token, mio::Ready::empty(), poll_opts)?;
         }
+
+        poll.reregister(
+            self.child_watcher.event_rx(),
+            self.child_event_token,
+            mio::Ready::readable(),
+            poll_opts,
+        )?;
+
         Ok(())
     }
 
@@ -271,6 +268,7 @@ impl<'a> EventedReadWrite for Pty<'a> {
     fn deregister(&mut self, poll: &mio::Poll) -> io::Result<()> {
         poll.deregister(&self.conout)?;
         poll.deregister(&self.conin)?;
+        poll.deregister(self.child_watcher.event_rx())?;
         Ok(())
     }
 
@@ -295,4 +293,16 @@ impl<'a> EventedReadWrite for Pty<'a> {
     }
 }
 
-impl<'a> EventedPty for Pty<'a> {}
+impl<'a> EventedPty for Pty<'a> {
+    fn child_event_token(&self) -> mio::Token {
+        self.child_event_token
+    }
+
+    fn next_child_event(&mut self) -> Option<ChildEvent> {
+        match self.child_watcher.event_rx().try_recv() {
+            Ok(ev) => Some(ev),
+            Err(TryRecvError::Empty) => None,
+            Err(TryRecvError::Disconnected) => Some(ChildEvent::Exited),
+        }
+    }
+}
diff --git a/alacritty_terminal/src/tty/windows/winpty.rs b/alacritty_terminal/src/tty/windows/winpty.rs
index db397ad9..258b7b2d 100644
--- a/alacritty_terminal/src/tty/windows/winpty.rs
+++ b/alacritty_terminal/src/tty/windows/winpty.rs
@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use super::{Pty, HANDLE};
-
 use std::fs::OpenOptions;
 use std::io;
 use std::os::windows::fs::OpenOptionsExt;
@@ -30,6 +28,8 @@ use winpty::{Config as WinptyConfig, ConfigFlags, MouseMode, SpawnConfig, SpawnF
 use crate::config::{Config, Shell};
 use crate::event::OnResize;
 use crate::term::SizeInfo;
+use crate::tty::windows::child::ChildExitWatcher;
+use crate::tty::windows::Pty;
 
 // We store a raw pointer because we need mutable access to call
 // on_resize from a separate thread. Winpty internally uses a mutex
@@ -136,10 +136,7 @@ pub fn new<'a, C>(config: &Config<C>, size: &SizeInfo, _window_id: Option<usize>
 
     winpty.spawn(&spawnconfig).unwrap();
 
-    unsafe {
-        HANDLE = winpty.raw_handle();
-    }
-
+    let child_watcher = ChildExitWatcher::new(winpty.raw_handle()).unwrap();
     let agent = Agent::new(winpty);
 
     Pty {
@@ -148,6 +145,8 @@ pub fn new<'a, C>(config: &Config<C>, size: &SizeInfo, _window_id: Option<usize>
         conin: super::EventedWritablePipe::Named(conin_pipe),
         read_token: 0.into(),
         write_token: 0.into(),
+        child_event_token: 0.into(),
+        child_watcher,
     }
 }