From 9d20fd5576ff97a0fde5f426147c949e51d29a73 Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Sun, 1 Mar 2020 17:56:58 +0200 Subject: [PATCH] Save forked processes for reaping --- src/components/mail/compose.rs | 29 ++++++----- src/components/mail/view.rs | 74 ++++++++++++++++++---------- src/components/mail/view/envelope.rs | 30 ++++++++--- src/components/mail/view/html.rs | 19 +++++-- src/components/notifications.rs | 17 ++++--- src/conf/accounts.rs | 7 ++- src/state.rs | 16 +++++- 7 files changed, 137 insertions(+), 55 deletions(-) diff --git a/src/components/mail/compose.rs b/src/components/mail/compose.rs index 089d5420e..204cf0ad4 100644 --- a/src/components/mail/compose.rs +++ b/src/components/mail/compose.rs @@ -912,21 +912,26 @@ impl Component for Composer { let parts = split_command!(editor); let (cmd, args) = (parts[0], &parts[1..]); - if let Err(e) = Command::new(cmd) + match Command::new(cmd) .args(args) .arg(&f.path()) .stdin(Stdio::inherit()) .stdout(Stdio::inherit()) - .output() + .spawn() { - context.replies.push_back(UIEvent::Notification( - Some(format!("Failed to execute {}", editor)), - e.to_string(), - Some(NotificationType::ERROR), - )); - context.replies.push_back(UIEvent::Fork(ForkType::Finished)); - context.restore_input(); - return true; + Ok(mut child) => { + let _ = child.wait(); + } + Err(err) => { + context.replies.push_back(UIEvent::Notification( + Some(format!("Failed to execute {}: {}", editor, err)), + err.to_string(), + Some(NotificationType::ERROR), + )); + context.replies.push_back(UIEvent::Fork(ForkType::Finished)); + context.restore_input(); + return true; + } } context.replies.push_back(UIEvent::Fork(ForkType::Finished)); let result = f.read_to_string(); @@ -973,7 +978,7 @@ impl Component for Composer { .spawn() { Ok(child) => { - let out = child + let _ = child .wait_with_output() .expect("failed to launch cmd") .stdout; @@ -1005,7 +1010,7 @@ impl Component for Composer { Err(err) => { context.replies.push_back(UIEvent::Notification( None, - format!("could not execute pipe cmd: {}", cmd), + format!("could not execute pipe cmd {}: {}", cmd, err), Some(NotificationType::ERROR), )); return true; diff --git a/src/components/mail/view.rs b/src/components/mail/view.rs index 57aa21823..3813acd05 100644 --- a/src/components/mail/view.rs +++ b/src/components/mail/view.rs @@ -1117,15 +1117,26 @@ impl Component for MailView { } _ => {} } - Command::new(&binary) + match Command::new(&binary) .arg(p.path()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() - .unwrap_or_else(|_| { - panic!("Failed to start {}", binary.display()) - }); - context.temp_files.push(p); + { + Ok(child) => { + context.temp_files.push(p); + context.children.push(child); + } + Err(err) => { + context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage(format!( + "Failed to start {}: {}", + binary.display(), + err + )), + )); + } + } } else { context.replies.push_back(UIEvent::StatusEvent( StatusEvent::DisplayMessage(if name.is_some() { @@ -1217,17 +1228,22 @@ impl Component for MailView { } }; - if let Err(e) = Command::new("xdg-open") + match Command::new("xdg-open") .arg(url) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() { - context.replies.push_back(UIEvent::Notification( - Some("Failed to launch xdg-open".to_string()), - e.to_string(), - Some(NotificationType::ERROR), - )); + Ok(child) => { + context.children.push(child); + } + Err(err) => { + context.replies.push_back(UIEvent::Notification( + Some("Failed to launch xdg-open".to_string()), + err.to_string(), + Some(NotificationType::ERROR), + )); + } } return true; } @@ -1427,18 +1443,23 @@ impl Component for MailView { } } list_management::ListAction::Url(url) => { - if let Err(e) = Command::new("xdg-open") + match Command::new("xdg-open") .arg(String::from_utf8_lossy(url).into_owned()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() { - context.replies.push_back(UIEvent::StatusEvent( - StatusEvent::DisplayMessage(format!( - "Couldn't launch xdg-open: {}", - e - )), - )); + Ok(child) => { + context.children.push(child); + } + Err(err) => { + context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage(format!( + "Couldn't launch xdg-open: {}", + err + )), + )); + } } return true; } @@ -1447,18 +1468,21 @@ impl Component for MailView { } MailingListAction::ListArchive if actions.archive.is_some() => { /* open archive url with xdg-open */ - if let Err(e) = Command::new("xdg-open") + match Command::new("xdg-open") .arg(actions.archive.unwrap()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() { - context.replies.push_back(UIEvent::StatusEvent( - StatusEvent::DisplayMessage(format!( - "Couldn't launch xdg-open: {}", - e - )), - )); + Ok(child) => context.children.push(child), + Err(err) => { + context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage(format!( + "Couldn't launch xdg-open: {}", + err + )), + )); + } } return true; } diff --git a/src/components/mail/view/envelope.rs b/src/components/mail/view/envelope.rs index 2e8531b5a..1b5130cb8 100644 --- a/src/components/mail/view/envelope.rs +++ b/src/components/mail/view/envelope.rs @@ -454,15 +454,26 @@ impl Component for EnvelopeView { None, true, ); - Command::new(&binary) + match Command::new(&binary) .arg(p.path()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() - .unwrap_or_else(|_| { - panic!("Failed to start {}", binary.display()) - }); - context.temp_files.push(p); + { + Ok(child) => { + context.children.push(child); + context.temp_files.push(p); + } + Err(err) => { + context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage(format!( + "Failed to start {}: {}", + binary.display(), + err + )), + )); + } + } } else { context.replies.push_back(UIEvent::StatusEvent( StatusEvent::DisplayMessage(format!( @@ -528,12 +539,17 @@ impl Component for EnvelopeView { } }; - Command::new("xdg-open") + match Command::new("xdg-open") .arg(url) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() - .expect("Failed to start xdg_open"); + { + Ok(child) => context.children.push(child), + Err(_err) => context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage("Failed to start xdg_open".into()), + )), + } return true; } UIEvent::Input(Key::Char('u')) => { diff --git a/src/components/mail/view/html.rs b/src/components/mail/view/html.rs index cc218fd51..8f600c1dd 100644 --- a/src/components/mail/view/html.rs +++ b/src/components/mail/view/html.rs @@ -137,13 +137,26 @@ impl Component for HtmlView { let binary = query_default_app("text/html"); if let Ok(binary) = binary { let p = create_temp_file(&self.bytes, None, None, true); - Command::new(&binary) + match Command::new(&binary) .arg(p.path()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() - .unwrap_or_else(|_| panic!("Failed to start {}", binary.display())); - context.temp_files.push(p); + { + Ok(child) => { + context.temp_files.push(p); + context.children.push(child); + } + Err(err) => { + context.replies.push_back(UIEvent::StatusEvent( + StatusEvent::DisplayMessage(format!( + "Failed to start {}: {}", + binary.display(), + err + )), + )); + } + } } else { context .replies diff --git a/src/components/notifications.rs b/src/components/notifications.rs index 5dfd74c4b..3d45d9f34 100644 --- a/src/components/notifications.rs +++ b/src/components/notifications.rs @@ -165,18 +165,23 @@ impl Component for NotificationFilter { fn process_event(&mut self, event: &mut UIEvent, context: &mut Context) -> bool { if let UIEvent::Notification(ref title, ref body, ref kind) = event { if let Some(ref bin) = context.runtime_settings.notifications.script { - if let Err(err) = Command::new(bin) + match Command::new(bin) .arg(title.as_ref().map(String::as_str).unwrap_or("meli")) .arg(body) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() { - log( - format!("Could not run notification script: {}.", err.to_string()), - ERROR, - ); - debug!("{:?}", err); + Ok(child) => { + context.children.push(child); + } + Err(err) => { + log( + format!("Could not run notification script: {}.", err.to_string()), + ERROR, + ); + debug!("{:?}", err); + } } } diff --git a/src/conf/accounts.rs b/src/conf/accounts.rs index b93a578e7..c8a7ae202 100644 --- a/src/conf/accounts.rs +++ b/src/conf/accounts.rs @@ -627,7 +627,7 @@ impl Account { if let Some(ref refresh_command) = self.settings.conf().refresh_command { let parts = crate::split_command!(refresh_command); let (cmd, args) = (parts[0], &parts[1..]); - std::process::Command::new(cmd) + let child = std::process::Command::new(cmd) .args(args) .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) @@ -638,6 +638,11 @@ impl Account { StatusEvent::DisplayMessage(format!("Running command {}", refresh_command)), ))) .unwrap(); + self.sender + .send(ThreadEvent::UIEvent(UIEvent::Fork( + crate::ForkType::Generic(child), + ))) + .unwrap(); return Ok(()); } diff --git a/src/state.rs b/src/state.rs index 014460cc4..c4e9cd6ec 100644 --- a/src/state.rs +++ b/src/state.rs @@ -103,6 +103,7 @@ pub struct Context { receiver: Receiver, input: InputHandler, work_controller: WorkController, + pub children: Vec, pub plugin_manager: PluginManager, pub temp_files: Vec, @@ -216,8 +217,16 @@ impl Drop for State { fn drop(&mut self) { // When done, restore the defaults to avoid messing with the terminal. self.switch_to_main_screen(); + use nix::sys::wait::{waitpid, WaitPidFlag}; + for child in self.context.children.iter_mut() { + if let Err(err) = waitpid( + nix::unistd::Pid::from_raw(child.id() as i32), + Some(WaitPidFlag::WNOHANG), + ) { + debug!("Failed to wait on subprocess {}: {}", child.id(), err); + } + } if let Some(ForkType::Embed(child_pid)) = self.child.take() { - use nix::sys::wait::{waitpid, WaitPidFlag}; /* Try wait, we don't want to block */ if let Err(e) = waitpid(child_pid, Some(WaitPidFlag::WNOHANG)) { debug!("Failed to wait on subprocess {}: {}", child_pid, e); @@ -329,6 +338,7 @@ impl State { replies: VecDeque::with_capacity(5), temp_files: Vec::new(), work_controller, + children: vec![], plugin_manager, sender, @@ -911,6 +921,10 @@ impl State { self.context.restore_input(); return; } + UIEvent::Fork(ForkType::Generic(child)) => { + self.context.children.push(child); + return; + } UIEvent::Fork(child) => { self.mode = UIMode::Fork; self.child = Some(child);