Make script-vars use process groups (#40)

* Make script-vars use process groups

Tail script-vars now make use of process groups to fix issues with
non-finished processes staying around.
I don't really understand any of this.

Cargo.lock

@@ -303,6 +303,7 @@ dependencies = [
  "debug_stub_derive",
  "derive_more",
  "extend",
+ "filedescriptor",
  "gdk",
  "gdk-pixbuf",
  "gdkx11",
@@ -345,6 +346,17 @@ dependencies = [
  "syn 1.0.44",
 ]
 
+[[package]]
+name = "filedescriptor"
+version = "0.7.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e70cb4dda2f343f3b7a98a6536559d04a700136cada190822e5d6a99e4184c06"
+dependencies = [
+ "anyhow",
+ "libc",
+ "winapi 0.3.9",
+]
+
 [[package]]
 name = "filetime"
 version = "0.2.12"

Cargo.toml

@@ -44,6 +44,7 @@ ref-cast = "1.0"
 popol = "0.3"
 nix = "0.19"
 smart-default = "0.6"
+filedescriptor = "0.7"
 
 [dev-dependencies]
 pretty_assertions = "0.6.1"

src/app.rs

@@ -103,6 +103,8 @@ impl App {
         // remove and close existing window with the same name
         let _ = self.close_window(window_name);
 
+        log::info!("Opening window {}", window_name);
+
         let mut window_def = self
             .eww_config
             .get_windows()
@@ -177,6 +179,7 @@ impl App {
     }
 
     pub fn reload_all_windows(&mut self, config: config::EwwConfig) -> Result<()> {
+        log::info!("Reloading windows");
         // refresh script-var poll stuff
         util::print_result_err(
             "while setting up script-var commands",

src/main.rs

@@ -113,20 +113,21 @@ pub enum OptAction {
 
 impl OptAction {
     fn into_eww_command(self) -> (app::EwwCommand, Option<crossbeam_channel::Receiver<String>>) {
-        match self {
-            OptAction::Update { fieldname, value } => (app::EwwCommand::UpdateVar(fieldname, value), None),
-            OptAction::OpenWindow { window_name, pos, size } => (app::EwwCommand::OpenWindow { window_name, pos, size }, None),
-            OptAction::CloseWindow { window_name } => (app::EwwCommand::CloseWindow { window_name }, None),
-            OptAction::KillServer => (app::EwwCommand::KillServer, None),
+        let command = match self {
+            OptAction::Update { fieldname, value } => app::EwwCommand::UpdateVar(fieldname, value),
+            OptAction::OpenWindow { window_name, pos, size } => app::EwwCommand::OpenWindow { window_name, pos, size },
+            OptAction::CloseWindow { window_name } => app::EwwCommand::CloseWindow { window_name },
+            OptAction::KillServer => app::EwwCommand::KillServer,
             OptAction::ShowState => {
                 let (send, recv) = crossbeam_channel::unbounded();
-                (app::EwwCommand::PrintState(send), Some(recv))
+                return (app::EwwCommand::PrintState(send), Some(recv));
             }
             OptAction::ShowDebug => {
                 let (send, recv) = crossbeam_channel::unbounded();
-                (app::EwwCommand::PrintDebug(send), Some(recv))
+                return (app::EwwCommand::PrintDebug(send), Some(recv));
             }
-        }
+        };
+        (command, None)
     }
 
     fn is_server_command(&self) -> bool {
@@ -163,7 +164,7 @@ fn try_main() -> Result<()> {
 }
 
 fn initialize_server(opts: Opt) -> Result<()> {
-    if opts.action == OptAction::KillServer || !opts.action.is_server_command() {
+    if !opts.action.is_server_command() {
         println!("No eww server running");
         return Ok(());
     }
@@ -182,6 +183,7 @@ fn initialize_server(opts: Opt) -> Result<()> {
     gtk::init()?;
     let (evt_send, evt_recv) = glib::MainContext::channel(glib::PRIORITY_DEFAULT);
 
+    log::info!("Initializing script var handler");
     let mut script_var_handler = script_var_handler::ScriptVarHandler::new(evt_send.clone())?;
     script_var_handler.initialize_clean(eww_config.get_script_vars().clone())?;
 
@@ -203,6 +205,7 @@ fn initialize_server(opts: Opt) -> Result<()> {
     }
 
     // run the command that eww was started with
+    log::info!("running command: {:?}", &opts.action);
     let (command, maybe_response_recv) = opts.action.into_eww_command();
     app.handle_command(command);
     if let Some(response_recv) = maybe_response_recv {
@@ -292,6 +295,8 @@ fn do_detach() -> Result<()> {
         nix::unistd::ForkResult::Child => {}
     }
 
+    nix::unistd::setsid().context("Failed to run setsid")?;
+
     let file = std::fs::OpenOptions::new()
         .create(true)
         .append(true)
@@ -308,7 +313,6 @@ fn do_detach() -> Result<()> {
     if nix::unistd::isatty(2)? {
         nix::unistd::dup2(std::io::stderr().as_raw_fd(), fd)?;
     }
-    nix::unistd::close(fd)?;
     Ok(())
 }
 

src/script_var_handler.rs

@@ -1,9 +1,4 @@
-use std::{
-    collections::HashMap,
-    io::BufReader,
-    process::{Child, Stdio},
-    time::Duration,
-};
+use std::{collections::HashMap, ffi::CString, io::BufReader, time::Duration};
 
 use crate::{app, config, eww_state, util, value::PrimitiveValue};
 use anyhow::*;
@@ -11,7 +6,7 @@ use app::EwwCommand;
 use glib;
 use itertools::Itertools;
 use scheduled_executor;
-use std::io::BufRead;
+use std::{io::BufRead, os::unix::io::AsRawFd};
 
 /// Handler that manages running and updating [ScriptVar]s
 pub struct ScriptVarHandler {
@@ -54,6 +49,7 @@ impl ScriptVarHandler {
         }
         self.setup_poll_tasks(&poll_script_vars)?;
         self.setup_tail_tasks(&tail_script_vars)?;
+        log::info!("Finished initializing script-var-handler");
         Ok(())
     }
 
@@ -80,6 +76,7 @@ impl ScriptVarHandler {
                 )
             })
             .collect_vec();
+        log::info!("finished setting up poll tasks");
         Ok(())
     }
 
@@ -88,13 +85,18 @@ impl ScriptVarHandler {
         log::info!("initializing handler for tail script vars");
         let mut sources = popol::Sources::with_capacity(tail_script_vars.len());
 
+        // TODO clean up this unnecessary vec, it really should not be needed.
+        // should be possibel to just keep a BufReader in TailVarProcess directly
         let mut command_children = Vec::new();
-        let mut command_out_handles = HashMap::new();
+        let mut command_out_handles: HashMap<_, BufReader<filedescriptor::FileDescriptor>> = HashMap::new();
 
         for var in tail_script_vars {
-            if let Some(mut child) = try_run_command(&var.command) {
-                command_out_handles.insert(var.name.clone(), BufReader::new(child.stdout.take().unwrap()));
-                command_children.push(child);
+            match TailVarProcess::run(&var.command) {
+                Ok(process) => {
+                    command_out_handles.insert(var.name.clone(), BufReader::new(process.out_fd.try_clone()?));
+                    command_children.push(process);
+                }
+                Err(err) => eprintln!("Failed to launch script-var command for tail: {:?}", err),
             }
         }
 
@@ -130,9 +132,7 @@ impl ScriptVarHandler {
             }
 
             // stop child processes after exit
-            for mut child in command_children {
-                let _ = child.kill();
-            }
+            command_children.drain(..).for_each(|process| process.kill());
         });
         self.tail_handler_thread = Some(thread_handle);
         Ok(())
@@ -145,23 +145,42 @@ impl Drop for ScriptVarHandler {
     }
 }
 
-/// Run a command in sh, returning its stdout-handle wrapped in a
-/// [`BufReader`]. If running the command fails, will print a warning
-/// and return `None`.
-fn try_run_command(command: &str) -> Option<Child> {
-    let result = std::process::Command::new("sh")
-        .arg("-c")
-        .arg(command)
-        .stdout(Stdio::piped())
-        .stderr(Stdio::inherit())
-        .stdin(Stdio::null())
-        .spawn();
+#[derive(Debug)]
+struct TailVarProcess {
+    pid: nix::unistd::Pid,
+    out_fd: filedescriptor::FileDescriptor,
+}
 
-    match result {
-        Ok(handle) => Some(handle),
-        Err(err) => {
-            eprintln!("WARN: Error running command from script-variable: {:?}", err);
-            None
+impl TailVarProcess {
+    pub fn run(command: &str) -> Result<Self> {
+        use nix::unistd::*;
+
+        let pipe = filedescriptor::Pipe::new()?;
+
+        match unsafe { fork()? } {
+            ForkResult::Child => {
+                std::mem::drop(pipe.read);
+                dup2(pipe.write.as_raw_fd(), std::io::stdout().as_raw_fd())?;
+                setpgid(Pid::from_raw(0), Pid::from_raw(0))?;
+                execv(
+                    CString::new("/bin/sh")?.as_ref(),
+                    &[CString::new("/bin/sh")?, CString::new("-c")?, CString::new(command)?],
+                )?;
+                unreachable!("Child fork called exec, thus the process was replaced by the command the user provided");
+            }
+            ForkResult::Parent { child, .. } => {
+                std::mem::drop(pipe.write);
+                setpgid(child, child)?;
+                Ok(TailVarProcess {
+                    pid: child,
+                    out_fd: pipe.read,
+                })
+            }
         }
     }
+
+    pub fn kill(self) {
+        let result = nix::sys::signal::kill(self.pid, Some(nix::sys::signal::SIGTERM));
+        util::print_result_err("Killing tail-var process", &result);
+    }
 }