Merge branch 'windows-fix-openvpn-shutdown'

5 files changed, 123 insertions, 86 deletions

diff --git a/talpid-core/src/process/mod.rs b/talpid-core/src/process/mod.rs
index 88bdc12ea4..1613389e4f 100644
--- a/talpid-core/src/process/mod.rs
+++ b/talpid-core/src/process/mod.rs
@@ -1,6 +1,5 @@
 /// A module for all OpenVPN related process management.
 pub mod openvpn;
 
-/// Unix specific process management features.
-#[cfg(unix)]
-pub mod unix;
+/// A trait for stopping subprocesses gracefully.
+pub mod stoppable_process;
diff --git a/talpid-core/src/process/openvpn.rs b/talpid-core/src/process/openvpn.rs
index bf7d1ef477..805d97c0a6 100644
--- a/talpid-core/src/process/openvpn.rs
+++ b/talpid-core/src/process/openvpn.rs
@@ -1,11 +1,17 @@
-use atty;
 use duct;
+extern crate libc;
+extern crate os_pipe;
 
+use super::stoppable_process::StoppableProcess;
 use std::ffi::{OsStr, OsString};
 use std::fmt;
 use std::path::{Path, PathBuf};
+use std::sync::Mutex;
 
+use self::os_pipe::{pipe, PipeWriter};
+use atty;
 use shell_escape;
+use std::io;
 use talpid_types::net;
 
 static BASE_ARGUMENTS: &[&[&str]] = &[
@@ -107,23 +113,7 @@ impl OpenVpnCommand {
     /// Build a runnable expression from the current state of the command.
     pub fn build(&self) -> duct::Expression {
         debug!("Building expression: {}", &self);
-
-        let mut cmd = duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked();
-
-        // Prevent forwarding the stdio when it's not available.
-        if !atty::is(atty::Stream::Stdin) {
-            cmd = cmd.stdin_null();
-        }
-
-        if !atty::is(atty::Stream::Stdout) {
-            cmd = cmd.stdout_null();
-        }
-
-        if !atty::is(atty::Stream::Stderr) {
-            cmd = cmd.stderr_null();
-        }
-
-        cmd
+        duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked()
     }
 
     /// Sets extra options
@@ -229,6 +219,58 @@ impl fmt::Display for OpenVpnCommand {
     }
 }
 
+/// Proc handle for an openvpn process
+pub struct OpenVpnProcHandle {
+    /// Duct handle
+    pub inner: duct::Handle,
+    /// Standard input handle
+    pub stdin: Mutex<Option<PipeWriter>>,
+}
+
+/// Impl for proc handle
+impl OpenVpnProcHandle {
+    /// Constructor for a new openvpn proc handle
+    pub fn new(mut cmd: duct::Expression) -> io::Result<Self> {
+        if !atty::is(atty::Stream::Stdout) {
+            cmd = cmd.stdout_null();
+        }
+
+        if !atty::is(atty::Stream::Stderr) {
+            cmd = cmd.stderr_null();
+        }
+
+        let (reader, writer) = pipe()?;
+        let proc_handle = cmd.stdin_handle(reader).start()?;
+
+        Ok(Self {
+            inner: proc_handle,
+            stdin: Mutex::new(Some(writer)),
+        })
+    }
+}
+
+impl StoppableProcess for OpenVpnProcHandle {
+    /// Closes STDIN to stop the openvpn process
+    fn stop(&self) {
+        let mut stdin = self.stdin.lock().unwrap();
+        // Dropping our stdin handle so that it is closed once. Closing the handle should
+        // gracefully stop our openvpn child process.
+        let _ = stdin.take();
+    }
+
+    fn kill(&self) -> io::Result<()> {
+        self.inner.kill()
+    }
+
+    fn has_stopped(&self) -> io::Result<bool> {
+        match self.inner.try_wait() {
+            Ok(None) => Ok(false),
+            Ok(Some(_)) => Ok(true),
+            Err(e) => Err(e),
+        }
+    }
+}
+
 
 #[cfg(test)]
 mod tests {
diff --git a/talpid-core/src/process/stoppable_process.rs b/talpid-core/src/process/stoppable_process.rs
new file mode 100644
index 0000000000..bfd0ec3ac4
--- /dev/null
+++ b/talpid-core/src/process/stoppable_process.rs
@@ -0,0 +1,52 @@
+extern crate libc;
+
+use std::io;
+use std::thread;
+use std::time::{Duration, Instant};
+
+static POLL_INTERVAL_MS: Duration = Duration::from_millis(50);
+
+/// A best effort attempt at stopping a subprocess whilst also ensuring that the subprocess is
+/// killed eventually.
+pub trait StoppableProcess
+where
+    Self: Sized,
+{
+    /// Gracefully stops a process.
+    fn stop(&self);
+
+    /// Kills a process unconditionally. Implementations should strive to never fail.
+    fn kill(&self) -> io::Result<()>;
+
+    /// Check if process is stopped. This method must not block.
+    fn has_stopped(&self) -> io::Result<bool>;
+
+    /// Attempts to stop a process gracefully in the given time period, otherwise kills the
+    /// process.
+    fn nice_kill(&self, timeout: Duration) -> io::Result<()> {
+        trace!("Trying to stop child process gracefully");
+        self.stop();
+        if wait_timeout(self, timeout)? {
+            debug!("Child process terminated gracefully");
+            Ok(())
+        } else {
+            warn!("Child process did not terminate gracefully within timeout, forcing termination");
+            self.kill()
+        }
+    }
+}
+/// Wait for a process to die for a maximum of `timeout`. Returns true if the process died within
+/// the timeout.
+fn wait_timeout<T>(process: &T, timeout: Duration) -> io::Result<bool>
+where
+    T: StoppableProcess + Sized,
+{
+    let timer = Instant::now();
+    while timer.elapsed() < timeout {
+        if process.has_stopped()? {
+            return Ok(true);
+        }
+        thread::sleep(POLL_INTERVAL_MS);
+    }
+    Ok(false)
+}
diff --git a/talpid-core/src/process/unix.rs b/talpid-core/src/process/unix.rs
deleted file mode 100644
index 4dea0f5a79..0000000000
--- a/talpid-core/src/process/unix.rs
+++ /dev/null
@@ -1,46 +0,0 @@
-extern crate libc;
-
-use duct;
-use duct::unix::HandleExt;
-
-use std::io;
-use std::thread;
-use std::time::{Duration, Instant};
-
-static POLL_INTERVAL_MS: u64 = 50;
-
-/// Extra methods for terminating `duct::Handle` instances.
-pub trait HandleKillExt {
-    /// Kills a process by first sending it the `SIGTERM` signal and then wait up to `timeout`.
-    /// If the process has not died after the timeout has expired it is killed.
-    fn nice_kill(&self, timeout: Duration) -> io::Result<()>;
-}
-
-impl HandleKillExt for duct::Handle {
-    fn nice_kill(&self, timeout: Duration) -> io::Result<()> {
-        trace!("Sending SIGTERM to child process");
-        self.send_signal(libc::SIGTERM)?;
-
-        if wait_timeout(self, timeout)? {
-            debug!("Child process exited from SIGTERM");
-            Ok(())
-        } else {
-            warn!("Child process did not exit from SIGTERM, sending SIGKILL");
-            self.kill()
-        }
-    }
-}
-
-/// Wait for a process to die for a maximum of `timeout`. Returns true if the process died within
-/// the timeout.
-fn wait_timeout(handle: &duct::Handle, timeout: Duration) -> io::Result<bool> {
-    let timer = Instant::now();
-    while timer.elapsed() < timeout {
-        match handle.try_wait() {
-            Ok(None) => thread::sleep(Duration::from_millis(POLL_INTERVAL_MS)),
-            Ok(Some(_)) => return Ok(true),
-            Err(e) => return Err(e),
-        }
-    }
-    Ok(false)
-}
diff --git a/talpid-core/src/tunnel/openvpn.rs b/talpid-core/src/tunnel/openvpn.rs
index 34b6979d77..58fa15eca2 100644
--- a/talpid-core/src/tunnel/openvpn.rs
+++ b/talpid-core/src/tunnel/openvpn.rs
@@ -1,6 +1,6 @@
-use duct;
 use openvpn_plugin::types::OpenVpnPluginEvent;
-use process::openvpn::OpenVpnCommand;
+use process::openvpn::{OpenVpnCommand, OpenVpnProcHandle};
+use process::stoppable_process::StoppableProcess;
 
 use std::collections::HashMap;
 use std::io;
@@ -9,7 +9,6 @@ use std::process::ExitStatus;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{mpsc, Arc};
 use std::thread;
-#[cfg(unix)]
 use std::time::Duration;
 
 use talpid_ipc;
@@ -32,9 +31,7 @@ mod errors {
 pub use self::errors::*;
 
 
-#[cfg(unix)]
-static OPENVPN_DIE_TIMEOUT: Duration = Duration::from_secs(2);
-
+static OPENVPN_DIE_TIMEOUT: Duration = Duration::from_secs(4);
 
 /// Struct for monitoring an OpenVPN process.
 #[derive(Debug)]
@@ -150,7 +147,7 @@ impl<C: OpenVpnBuilder> OpenVpnMonitor<C> {
 
 /// A handle to an `OpenVpnMonitor` for closing it.
 #[derive(Debug, Clone)]
-pub struct OpenVpnCloseHandle<H: ProcessHandle = duct::Handle> {
+pub struct OpenVpnCloseHandle<H: ProcessHandle = OpenVpnProcHandle> {
     child: Arc<H>,
     closed: Arc<AtomicBool>,
 }
@@ -195,32 +192,25 @@ pub trait ProcessHandle: Send + Sync + 'static {
 }
 
 impl OpenVpnBuilder for OpenVpnCommand {
-    type ProcessHandle = duct::Handle;
+    type ProcessHandle = OpenVpnProcHandle;
 
     fn plugin<P: AsRef<Path>>(&mut self, path: P, args: Vec<String>) -> &mut Self {
         self.plugin(path, args)
     }
 
-    fn start(&self) -> io::Result<duct::Handle> {
-        self.build().start()
+    fn start(&self) -> io::Result<OpenVpnProcHandle> {
+        OpenVpnProcHandle::new(self.build())
     }
 }
 
-impl ProcessHandle for duct::Handle {
+impl ProcessHandle for OpenVpnProcHandle {
     fn wait(&self) -> io::Result<ExitStatus> {
-        self.wait().map(|output| output.status)
+        self.inner.wait().map(|output| output.status)
     }
 
-    #[cfg(unix)]
     fn kill(&self) -> io::Result<()> {
-        use process::unix::HandleKillExt;
         self.nice_kill(OPENVPN_DIE_TIMEOUT)
     }
-
-    #[cfg(not(unix))]
-    fn kill(&self) -> io::Result<()> {
-        duct::Handle::kill(self)
-    }
 }