Fix shutdown of config monitor

This implements a coordinated shutdown of the config monitor by sending
an event to its thread and waiting for the thread to terminate.

2 files changed, 143 insertions, 110 deletions

diff --git a/alacritty/src/config/monitor.rs b/alacritty/src/config/monitor.rs
index f4b39a22..53cff1c9 100644
--- a/alacritty/src/config/monitor.rs
+++ b/alacritty/src/config/monitor.rs
@@ -1,9 +1,13 @@
 use std::path::PathBuf;
-use std::sync::mpsc::{self, RecvTimeoutError};
+use std::sync::mpsc::{self, RecvTimeoutError, Sender};
+use std::thread::JoinHandle;
 use std::time::{Duration, Instant};
 
-use log::{debug, error};
-use notify::{Config, EventKind, RecommendedWatcher, RecursiveMode, Watcher};
+use log::{debug, error, warn};
+use notify::{
+    Config, Error as NotifyError, Event as NotifyEvent, EventKind, RecommendedWatcher,
+    RecursiveMode, Watcher,
+};
 use winit::event_loop::EventLoopProxy;
 
 use alacritty_terminal::thread;
@@ -15,115 +19,139 @@ const DEBOUNCE_DELAY: Duration = Duration::from_millis(10);
 /// The fallback for `RecommendedWatcher` polling.
 const FALLBACK_POLLING_TIMEOUT: Duration = Duration::from_secs(1);
 
-pub fn watch(mut paths: Vec<PathBuf>, event_proxy: EventLoopProxy<Event>) {
-    // Don't monitor config if there is no path to watch.
-    if paths.is_empty() {
-        return;
-    }
+/// Config file update monitor.
+pub struct ConfigMonitor {
+    thread: JoinHandle<()>,
+    shutdown_tx: Sender<Result<NotifyEvent, NotifyError>>,
+}
 
-    // Exclude char devices like `/dev/null`, sockets, and so on, by checking that file type is a
-    // regular file.
-    paths.retain(|path| {
-        // Call `metadata` to resolve symbolic links.
-        path.metadata().map_or(false, |metadata| metadata.file_type().is_file())
-    });
-
-    // Canonicalize paths, keeping the base paths for symlinks.
-    for i in 0..paths.len() {
-        if let Ok(canonical_path) = paths[i].canonicalize() {
-            match paths[i].symlink_metadata() {
-                Ok(metadata) if metadata.file_type().is_symlink() => paths.push(canonical_path),
-                _ => paths[i] = canonical_path,
-            }
+impl ConfigMonitor {
+    pub fn new(mut paths: Vec<PathBuf>, event_proxy: EventLoopProxy<Event>) -> Option<Self> {
+        // Don't monitor config if there is no path to watch.
+        if paths.is_empty() {
+            return None;
         }
-    }
 
-    // The Duration argument is a debouncing period.
-    let (tx, rx) = mpsc::channel();
-    let mut watcher = match RecommendedWatcher::new(
-        tx,
-        Config::default().with_poll_interval(FALLBACK_POLLING_TIMEOUT),
-    ) {
-        Ok(watcher) => watcher,
-        Err(err) => {
-            error!("Unable to watch config file: {}", err);
-            return;
-        },
-    };
-
-    thread::spawn_named("config watcher", move || {
-        // Get all unique parent directories.
-        let mut parents = paths
-            .iter()
-            .map(|path| {
-                let mut path = path.clone();
-                path.pop();
-                path
-            })
-            .collect::<Vec<PathBuf>>();
-        parents.sort_unstable();
-        parents.dedup();
-
-        // Watch all configuration file directories.
-        for parent in &parents {
-            if let Err(err) = watcher.watch(parent, RecursiveMode::NonRecursive) {
-                debug!("Unable to watch config directory {:?}: {}", parent, err);
+        // Exclude char devices like `/dev/null`, sockets, and so on, by checking that file type is
+        // a regular file.
+        paths.retain(|path| {
+            // Call `metadata` to resolve symbolic links.
+            path.metadata().map_or(false, |metadata| metadata.file_type().is_file())
+        });
+
+        // Canonicalize paths, keeping the base paths for symlinks.
+        for i in 0..paths.len() {
+            if let Ok(canonical_path) = paths[i].canonicalize() {
+                match paths[i].symlink_metadata() {
+                    Ok(metadata) if metadata.file_type().is_symlink() => paths.push(canonical_path),
+                    _ => paths[i] = canonical_path,
+                }
             }
         }
 
-        // The current debouncing time.
-        let mut debouncing_deadline: Option<Instant> = None;
-
-        // The events accumulated during the debounce period.
-        let mut received_events = Vec::new();
-
-        loop {
-            // We use `recv_timeout` to debounce the events coming from the watcher and reduce
-            // the amount of config reloads.
-            let event = match debouncing_deadline.as_ref() {
-                Some(debouncing_deadline) => {
-                    rx.recv_timeout(debouncing_deadline.saturating_duration_since(Instant::now()))
-                },
-                None => {
-                    let event = rx.recv().map_err(Into::into);
-                    // Set the debouncing deadline after receiving the event.
-                    debouncing_deadline = Some(Instant::now() + DEBOUNCE_DELAY);
-                    event
-                },
-            };
-
-            match event {
-                Ok(Ok(event)) => match event.kind {
-                    EventKind::Any
-                    | EventKind::Create(_)
-                    | EventKind::Modify(_)
-                    | EventKind::Other => {
-                        received_events.push(event);
+        // The Duration argument is a debouncing period.
+        let (tx, rx) = mpsc::channel();
+        let mut watcher = match RecommendedWatcher::new(
+            tx.clone(),
+            Config::default().with_poll_interval(FALLBACK_POLLING_TIMEOUT),
+        ) {
+            Ok(watcher) => watcher,
+            Err(err) => {
+                error!("Unable to watch config file: {}", err);
+                return None;
+            },
+        };
+
+        let join_handle = thread::spawn_named("config watcher", move || {
+            // Get all unique parent directories.
+            let mut parents = paths
+                .iter()
+                .map(|path| {
+                    let mut path = path.clone();
+                    path.pop();
+                    path
+                })
+                .collect::<Vec<PathBuf>>();
+            parents.sort_unstable();
+            parents.dedup();
+
+            // Watch all configuration file directories.
+            for parent in &parents {
+                if let Err(err) = watcher.watch(parent, RecursiveMode::NonRecursive) {
+                    debug!("Unable to watch config directory {:?}: {}", parent, err);
+                }
+            }
+
+            // The current debouncing time.
+            let mut debouncing_deadline: Option<Instant> = None;
+
+            // The events accumulated during the debounce period.
+            let mut received_events = Vec::new();
+
+            loop {
+                // We use `recv_timeout` to debounce the events coming from the watcher and reduce
+                // the amount of config reloads.
+                let event = match debouncing_deadline.as_ref() {
+                    Some(debouncing_deadline) => rx.recv_timeout(
+                        debouncing_deadline.saturating_duration_since(Instant::now()),
+                    ),
+                    None => {
+                        let event = rx.recv().map_err(Into::into);
+                        // Set the debouncing deadline after receiving the event.
+                        debouncing_deadline = Some(Instant::now() + DEBOUNCE_DELAY);
+                        event
+                    },
+                };
+
+                match event {
+                    Ok(Ok(event)) => match event.kind {
+                        EventKind::Other if event.info() == Some("shutdown") => break,
+                        EventKind::Any
+                        | EventKind::Create(_)
+                        | EventKind::Modify(_)
+                        | EventKind::Other => {
+                            received_events.push(event);
+                        },
+                        _ => (),
+                    },
+                    Err(RecvTimeoutError::Timeout) => {
+                        // Go back to polling the events.
+                        debouncing_deadline = None;
+
+                        if received_events
+                            .drain(..)
+                            .flat_map(|event| event.paths.into_iter())
+                            .any(|path| paths.contains(&path))
+                        {
+                            // Always reload the primary configuration file.
+                            let event = Event::new(EventType::ConfigReload(paths[0].clone()), None);
+                            let _ = event_proxy.send_event(event);
+                        }
                     },
-                    _ => (),
-                },
-                Err(RecvTimeoutError::Timeout) => {
-                    // Go back to polling the events.
-                    debouncing_deadline = None;
-
-                    if received_events
-                        .drain(..)
-                        .flat_map(|event| event.paths.into_iter())
-                        .any(|path| paths.contains(&path))
-                    {
-                        // Always reload the primary configuration file.
-                        let event = Event::new(EventType::ConfigReload(paths[0].clone()), None);
-                        let _ = event_proxy.send_event(event);
-                    }
-                },
-                Ok(Err(err)) => {
-                    debug!("Config watcher errors: {:?}", err);
-                },
-                Err(err) => {
-                    debug!("Config watcher channel dropped unexpectedly: {}", err);
-                    break;
-                },
-            };
+                    Ok(Err(err)) => {
+                        debug!("Config watcher errors: {:?}", err);
+                    },
+                    Err(err) => {
+                        debug!("Config watcher channel dropped unexpectedly: {}", err);
+                        break;
+                    },
+                };
+            }
+        });
+
+        Some(Self { thread: join_handle, shutdown_tx: tx })
+    }
+
+    /// Synchronously shut down the monitor.
+    pub fn shutdown(self) {
+        // Request shutdown.
+        let mut event = NotifyEvent::new(EventKind::Other);
+        event = event.set_info("shutdown");
+        let _ = self.shutdown_tx.send(Ok(event));
+
+        // Wait for thread to terminate.
+        if let Err(err) = self.thread.join() {
+            warn!("config monitor shutdown failed: {err:?}");
         }
-    });
+    }
 }
diff --git a/alacritty/src/main.rs b/alacritty/src/main.rs
index 2a60961a..f9301767 100644
--- a/alacritty/src/main.rs
+++ b/alacritty/src/main.rs
@@ -56,7 +56,8 @@ mod gl {
 #[cfg(unix)]
 use crate::cli::MessageOptions;
 use crate::cli::{Options, Subcommands};
-use crate::config::{monitor, UiConfig};
+use crate::config::monitor::ConfigMonitor;
+use crate::config::UiConfig;
 use crate::event::{Event, Processor};
 #[cfg(target_os = "macos")]
 use crate::macos::locale;
@@ -165,8 +166,10 @@ fn alacritty(mut options: Options) -> Result<(), Box<dyn Error>> {
     //
     // The monitor watches the config file for changes and reloads it. Pending
     // config changes are processed in the main loop.
+    let mut config_monitor = None;
     if config.live_config_reload {
-        monitor::watch(config.config_paths.clone(), window_event_loop.create_proxy());
+        config_monitor =
+            ConfigMonitor::new(config.config_paths.clone(), window_event_loop.create_proxy());
     }
 
     // Create the IPC socket listener.
@@ -205,8 +208,10 @@ fn alacritty(mut options: Options) -> Result<(), Box<dyn Error>> {
     // FIXME: Change PTY API to enforce the correct drop order with the typesystem.
     drop(processor);
 
-    // FIXME patch notify library to have a shutdown method.
-    // config_reloader.join().ok();
+    // Terminate the config monitor.
+    if let Some(config_monitor) = config_monitor.take() {
+        config_monitor.shutdown();
+    }
 
     // Without explicitly detaching the console cmd won't redraw it's prompt.
     #[cfg(windows)]