Fixup issues in minidumper (#28)

* Remove socket paths when Server is shutdown

* Add on_client_dis/connected methods

This adds handlers that make it easier for applications to exit the
server (and usually the watchdog process) when clients connect or
disconnect, eg a typical use case will be to have a single client
connected to the server, so once it disconnects the server can be
shutdown

* Fix mac

minidumper-test/src/lib.rs

@@ -119,7 +119,7 @@ pub fn spinup_server(id: &str) -> Server {
         fn on_minidump_created(
             &self,
             result: Result<minidumper::MinidumpBinary, minidumper::Error>,
-        ) -> bool {
+        ) -> minidumper::LoopAction {
             let md_bin = result.expect("failed to write minidump");
             md_bin
                 .file
@@ -132,7 +132,7 @@ pub fn spinup_server(id: &str) -> Server {
                 .send(md_bin.path)
                 .expect("couldn't send minidump path");
 
-            false
+            minidumper::LoopAction::Continue
         }
 
         fn on_message(&self, _kind: u32, _buffer: Vec<u8>) {

minidumper/examples/diskwrite.rs

@@ -27,7 +27,7 @@ fn main() {
             fn on_minidump_created(
                 &self,
                 result: Result<minidumper::MinidumpBinary, minidumper::Error>,
-            ) -> bool {
+            ) -> minidumper::LoopAction {
                 match result {
                     Ok(mut md_bin) => {
                         use std::io::Write;
@@ -39,7 +39,8 @@ fn main() {
                     }
                 }
 
-                true
+                // Tells the server to exit, which will in turn exit the process
+                minidumper::LoopAction::Exit
             }
 
             fn on_message(&self, kind: u32, buffer: Vec<u8>) {

minidumper/src/ipc/server.rs

@@ -1,5 +1,5 @@
 use super::{Connection, Header, Listener, SocketName};
-use crate::Error;
+use crate::{Error, LoopAction};
 use polling::{Event, Poller};
 use std::time::Duration;
 
@@ -9,6 +9,13 @@ pub struct Server {
     listener: Listener,
     #[cfg(target_os = "macos")]
     port: crash_context::ipc::Server,
+    /// For abstract sockets, we don't have to worry about cleanup as it is
+    /// handled by the OS, but on Windows and MacOS we need to clean them up
+    /// manually. We basically rely on the crash watchdog program this Server
+    /// is running in to exit cleanly, which should be mostly true, but we
+    /// may need to harden this code if people experience issues with socket
+    /// paths not being cleaned up reliably
+    socket_path: Option<std::path::PathBuf>,
 }
 
 struct ClientConn {
@@ -68,13 +75,16 @@ impl Server {
         let sn = name.into();
 
         #[allow(irrefutable_let_patterns)]
-        if let SocketName::Path(path) = &sn {
-            if path.exists() {
-                // We ignore the result, if we couldn't delete the file, it's
-                // most likely that the following bind will fail
-                let _res = std::fs::remove_file(path);
-            }
-        }
+        let socket_path = if let SocketName::Path(path) = &sn {
+            // There seems to be a bug, at least on Windows, where checking for
+            // the existence of the file path will actually fail even if the file
+            // is actually there, so we just unconditionally remove the path
+            let _res = std::fs::remove_file(path);
+
+            Some(std::path::PathBuf::from(path))
+        } else {
+            None
+        };
 
         cfg_if::cfg_if! {
             if #[cfg(any(target_os = "linux", target_os = "android"))] {
@@ -111,6 +121,7 @@ impl Server {
             listener,
             #[cfg(target_os = "macos")]
             port,
+            socket_path,
         })
     }
 
@@ -143,7 +154,7 @@ impl Server {
             poll.wait(&mut events, Some(Duration::from_millis(10)))?;
 
             #[cfg(target_os = "macos")]
-            if self.check_mach_port(&poll, &mut clients, handler.as_ref())? {
+            if self.check_mach_port(&poll, &mut clients, handler.as_ref())? == LoopAction::Exit {
                 return Ok(());
             }
 
@@ -163,6 +174,11 @@ impl Server {
                                 #[cfg(target_os = "macos")]
                                 pid: None,
                             });
+
+                            if handler.on_client_connected(clients.len()) == LoopAction::Exit {
+                                log::debug!("on_client_connected exited message loop");
+                                return Ok(());
+                            }
                         }
                         Err(err) => {
                             log::error!("failed to accept socket connection: {}", err);
@@ -227,23 +243,24 @@ impl Server {
                                         }
                                     }
 
-                                    let exit =
+                                    let action =
                                         match Self::handle_crash_request(crash_ctx, handler.as_ref()) {
                                             Err(err) => {
                                                 log::error!("failed to capture minidump: {}", err);
-                                                false
+                                                LoopAction::Continue
                                             }
-                                            Ok(exit) => {
+                                            Ok(action) => {
                                                 log::info!("captured minidump");
-                                                exit
+                                                action
                                             }
                                         };
 
                                     if let Err(e) = cc.socket.send(&[1]) {
                                         log::error!("failed to send ack: {}", e);
                                     }
 
-                                    if exit {
+                                    if action == LoopAction::Exit {
+                                        log::debug!("user handler requested exit after minidump creation");
                                         return Ok(());
                                     }
 
@@ -275,6 +292,11 @@ impl Server {
                         if let Err(e) = poll.delete(&socket) {
                             log::error!("failed to deregister socket: {}", e);
                         }
+
+                        if handler.on_client_disconnected(clients.len()) == LoopAction::Exit {
+                            log::debug!("on_client_disconnected exited message loop");
+                            return Ok(());
+                        }
                     } else {
                         poll.modify(&clients[pos].socket, Event::readable(clients[pos].key))?;
                     }
@@ -286,7 +308,7 @@ impl Server {
     fn handle_crash_request(
         crash_context: crash_context::CrashContext,
         handler: &dyn crate::ServerHandler,
-    ) -> Result<bool, Error> {
+    ) -> Result<LoopAction, Error> {
         cfg_if::cfg_if! {
             if #[cfg(any(target_os = "linux", target_os = "android"))] {
                 let mut writer =
@@ -323,7 +345,7 @@ impl Server {
         poll: &Poller,
         clients: &mut Vec<ClientConn>,
         handler: &dyn crate::ServerHandler,
-    ) -> Result<bool, Error> {
+    ) -> Result<LoopAction, Error> {
         // We use a really short timeout for receiving on the mach port since we check it
         // frequently rather than spawning a separate thread and blocking
         if let Some(mut rcc) = self
@@ -337,14 +359,14 @@ impl Server {
                 .ok_or(Error::UnknownClientPid)?;
             let cc = clients.swap_remove(pos);
 
-            let exit = match Self::handle_crash_request(rcc.crash_context, handler) {
+            let action = match Self::handle_crash_request(rcc.crash_context, handler) {
                 Err(err) => {
                     log::error!("failed to capture minidump: {}", err);
-                    false
+                    LoopAction::Continue
                 }
-                Ok(exit) => {
+                Ok(action) => {
                     log::info!("captured minidump");
-                    exit
+                    action
                 }
             };
 
@@ -356,9 +378,17 @@ impl Server {
                 log::error!("failed to deregister socket: {}", e);
             }
 
-            Ok(exit)
+            Ok(action)
         } else {
-            Ok(false)
+            Ok(LoopAction::Continue)
+        }
+    }
+}
+
+impl Drop for Server {
+    fn drop(&mut self) {
+        if let Some(path) = self.socket_path.take() {
+            let _res = std::fs::remove_file(path);
         }
     }
 }

minidumper/src/lib.rs

@@ -18,6 +18,17 @@ pub struct MinidumpBinary {
     pub contents: Vec<u8>,
 }
 
+/// Actions for the [`Server`] message loop to take after a [`ServerHandler`]
+/// method is invoked
+#[derive(Copy, Clone, PartialEq)]
+pub enum LoopAction {
+    /// Exits the message loop, stops processing messages, and disconnects all
+    /// connected clients
+    Exit,
+    /// Continues running the message loop as normal
+    Continue,
+}
+
 /// Allows user code to hook into the server to avoid hardcoding too many details
 pub trait ServerHandler: Send + Sync {
     /// Called when a crash request has been received and a backing file needs
@@ -28,7 +39,7 @@ pub trait ServerHandler: Send + Sync {
     ///
     /// A return value of true indicates that the message loop should exit and
     /// stop processing messages.
-    fn on_minidump_created(&self, result: Result<MinidumpBinary, Error>) -> bool;
+    fn on_minidump_created(&self, result: Result<MinidumpBinary, Error>) -> LoopAction;
     /// Called when the client sends a user message sent from the client with
     /// `send_message`
     fn on_message(&self, kind: u32, buffer: Vec<u8>);
@@ -38,4 +49,14 @@ pub trait ServerHandler: Send + Sync {
     fn message_alloc(&self) -> Vec<u8> {
         Vec::new()
     }
+    /// Called when a new client connection has been established with the Server,
+    /// with the number of currently active client connections.
+    fn on_client_connected(&self, _num_clients: usize) -> LoopAction {
+        LoopAction::Continue
+    }
+    /// Called when a client has disconnected from the Server, with the number
+    /// of currently active client connections.
+    fn on_client_disconnected(&self, _num_clients: usize) -> LoopAction {
+        LoopAction::Continue
+    }
 }

minidumper/tests/ipc.rs

@@ -26,7 +26,7 @@ fn ipc_messages() {
         fn on_minidump_created(
             &self,
             _result: Result<minidumper::MinidumpBinary, minidumper::Error>,
-        ) -> bool {
+        ) -> minidumper::LoopAction {
             panic!("should not be called");
         }