Ensure that the event loop thread cleanly exits on shutdown

Background:

If a shell process exits with children still alive (typically due to the
`disown` shell builtin), POLLHUP will not be sent to the master PTY file
descriptor. This is due to the fact that the disowned process still has
the slave PTY open as its STDIN, STDOUT, and STDERR.

If a disowned process never reads or writes from its file descriptors
(which is often the case for graphical applications), the event loop
will end up blocking on poll()/select() when not handling user input
received over the mio channel. When Alacritty shuts down and joins on the
event loop thread, there can never be any more input on the mio channel -
the main thread is no longer handling user keystrokes from the window. Unless
a disowned process happens to access its slave PTY file descriptors, the
event loop will never get the chance to deetect that it should exit.

This commit extends the `Msg` enum to include an explicit `Shutdown`
message, which ensures a clean shutdown (e.g. closing the 'recording'
file). This allows the select()/poll() call to remain blocking, instead
of needing to periodically check the shutdown state in between
timed-out calls.

Fixes #339

2 files changed, 31 insertions, 9 deletions

diff --git a/src/event_loop.rs b/src/event_loop.rs
index 8db5cddd..59fa4fbf 100644
--- a/src/event_loop.rs
+++ b/src/event_loop.rs
@@ -21,6 +21,9 @@ use sync::FairMutex;
 pub enum Msg {
     /// Data that should be written to the pty
     Input(Cow<'static, [u8]>),
+
+    /// Indicates that the `EvemtLoop` should shut down, as Alacritty is shutting down
+    Shutdown
 }
 
 /// The main event!.. loop.
@@ -169,24 +172,33 @@ impl<Io> EventLoop<Io>
 
     // Drain the channel
     //
-    // Returns true if items were received
-    fn drain_recv_channel(&self, state: &mut State) -> bool {
+    // Returns `Ok` if the `EventLoop` should continue running.
+    // `Ok(true)`is returned if items were received
+    //
+    // An `Err` indicates that the event loop should shut down
+    fn drain_recv_channel(&self, state: &mut State) -> Result<bool, ()> {
         let mut received_item = false;
         while let Ok(msg) = self.rx.try_recv() {
             received_item = true;
             match msg {
                 Msg::Input(input) => {
                     state.write_list.push_back(input);
+                },
+                Msg::Shutdown => {
+                    return Err(())
                 }
             }
         }
 
-        received_item
+        Ok(received_item)
     }
 
+    // Returns a `bool` indicating whether or not the event loop should continue running
     #[inline]
-    fn channel_event(&mut self, state: &mut State) {
-        self.drain_recv_channel(state);
+    fn channel_event(&mut self, state: &mut State) -> bool {
+        if self.drain_recv_channel(state).is_err() {
+            return false;
+        }
 
         self.poll.reregister(
             &self.rx, CHANNEL,
@@ -202,6 +214,7 @@ impl<Io> EventLoop<Io>
                 PollOpt::edge() | PollOpt::oneshot()
             ).expect("reregister fd after channel recv");
         }
+        true
     }
 
     #[inline]
@@ -241,9 +254,12 @@ impl<Io> EventLoop<Io>
                         //
                         // Doing this check in !terminal.dirty will prevent the
                         // condition from being checked overzealously.
+                        //
+                        // We want to break if `drain_recv_channel` returns either `Ok(true)`
+                        // (new items came in for writing) or `Err` (we need to shut down)
                         if state.writing.is_some()
                             || !state.write_list.is_empty()
-                            || self.drain_recv_channel(state)
+                            || self.drain_recv_channel(state).unwrap_or_else(|_| true)
                         {
                             break;
                         }
@@ -332,7 +348,11 @@ impl<Io> EventLoop<Io>
 
                 for event in events.iter() {
                     match event.token() {
-                        CHANNEL => self.channel_event(&mut state),
+                        CHANNEL =>  {
+                            if !self.channel_event(&mut state) {
+                                break 'event_loop;
+                            }
+                        },
                         PTY => {
                             let kind = event.kind();
 
diff --git a/src/main.rs b/src/main.rs
index 5126b09d..7c377f2b 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -29,7 +29,7 @@ use alacritty::cli;
 use alacritty::config::{self, Config};
 use alacritty::display::Display;
 use alacritty::event;
-use alacritty::event_loop::{self, EventLoop};
+use alacritty::event_loop::{self, EventLoop, Msg};
 use alacritty::logging;
 use alacritty::sync::FairMutex;
 use alacritty::term::{Term};
@@ -126,7 +126,7 @@ fn run(mut config: Config, options: cli::Options) -> Result<(), Box<Error>> {
     //
     // Need the Rc<RefCell<_>> here since a ref is shared in the resize callback
     let mut processor = event::Processor::new(
-        event_loop::Notifier(loop_tx),
+        event_loop::Notifier(event_loop.channel()),
         display.resize_channel(),
         &options,
         &config,
@@ -178,6 +178,8 @@ fn run(mut config: Config, options: cli::Options) -> Result<(), Box<Error>> {
         }
     }
 
+    loop_tx.send(Msg::Shutdown).expect("Error sending shutdown to event loop");
+
     // FIXME patch notify library to have a shutdown method
     // config_reloader.join().ok();