Merge branch 'weekly-cleanup/rewrite-talpid-openvpn-to-be-more-async-des-285' into main

3 files changed, 55 insertions, 86 deletions

diff --git a/talpid-openvpn/src/process/mod.rs b/talpid-openvpn/src/process/mod.rs
index 54cdeb9042..f496df31ae 100644
--- a/talpid-openvpn/src/process/mod.rs
+++ b/talpid-openvpn/src/process/mod.rs
@@ -1,6 +1,3 @@
 /// A module for all OpenVPN related process management.
 #[cfg(not(target_os = "android"))]
 pub mod openvpn;
-
-/// A trait for stopping subprocesses gracefully.
-pub mod stoppable_process;
diff --git a/talpid-openvpn/src/process/openvpn.rs b/talpid-openvpn/src/process/openvpn.rs
index 722468e3ca..44e00d0eda 100644
--- a/talpid-openvpn/src/process/openvpn.rs
+++ b/talpid-openvpn/src/process/openvpn.rs
@@ -1,6 +1,3 @@
-use duct;
-
-use super::stoppable_process::StoppableProcess;
 use os_pipe::{pipe, PipeWriter};
 use parking_lot::Mutex;
 use shell_escape;
@@ -190,9 +187,11 @@ impl OpenVpnCommand {
     }
 
     /// Build a runnable expression from the current state of the command.
-    pub fn build(&self) -> duct::Expression {
+    pub fn build(&self) -> tokio::process::Command {
         log::debug!("Building expression: {}", &self);
-        duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked()
+        let mut handle = tokio::process::Command::new(&self.openvpn_bin);
+        handle.args(self.get_arguments());
+        handle
     }
 
     /// Returns all arguments that the subprocess would be spawned with.
@@ -365,8 +364,11 @@ impl fmt::Display for OpenVpnCommand {
 
 /// Handle to a running OpenVPN process.
 pub struct OpenVpnProcHandle {
-    /// Duct handle
-    pub inner: duct::Handle,
+    /// Handle to the child process running OpenVPN.
+    ///
+    /// This handle is acquired by calling [`OpenVpnCommand::build`] (or
+    /// [`tokio::process::Command::spawn`]).
+    pub inner: std::sync::Arc<tokio::sync::Mutex<tokio::process::Child>>,
     /// Pipe handle to stdin of the OpenVPN process. Our custom fork of OpenVPN
     /// has been changed so that it exits cleanly when stdin is closed. This is a hack
     /// solution to cleanly shut OpenVPN down without using the
@@ -377,62 +379,85 @@ pub struct OpenVpnProcHandle {
 impl OpenVpnProcHandle {
     /// Configures the expression to run OpenVPN in a way compatible with this handle
     /// and spawns it. Returns the handle.
-    pub fn new(mut cmd: duct::Expression) -> io::Result<Self> {
-        use is_terminal::IsTerminal;
+    pub fn new(mut cmd: &mut tokio::process::Command) -> io::Result<Self> {
+        use std::io::IsTerminal;
 
         if !std::io::stdout().is_terminal() {
-            cmd = cmd.stdout_null();
+            cmd = cmd.stdout(std::process::Stdio::null())
         }
 
         if !std::io::stderr().is_terminal() {
-            cmd = cmd.stderr_null();
+            cmd = cmd.stderr(std::process::Stdio::null())
         }
 
         let (reader, writer) = pipe()?;
-        let proc_handle = cmd.stdin_file(reader).start()?;
+        let proc_handle = cmd.stdin(reader).spawn()?;
 
         Ok(Self {
-            inner: proc_handle,
+            inner: std::sync::Arc::new(tokio::sync::Mutex::new(proc_handle)),
             stdin: Mutex::new(Some(writer)),
         })
     }
-}
 
-impl StoppableProcess for OpenVpnProcHandle {
-    fn stop(&self) {
+    /// Attempts to stop the OpenVPN process gracefully in the given time
+    /// period, otherwise kills the process.
+    pub async fn nice_kill(&self, timeout: std::time::Duration) -> io::Result<()> {
+        log::debug!("Trying to stop child process gracefully");
+        self.stop().await;
+
+        // Wait for the process to die for a maximum of `timeout`.
+        let wait_result = tokio::time::timeout(timeout, self.wait()).await;
+        match wait_result {
+            Ok(_) => log::debug!("Child process terminated gracefully"),
+            Err(_) => {
+                log::warn!(
+                "Child process did not terminate gracefully within timeout, forcing termination"
+            );
+                self.kill().await?;
+            }
+        }
+        Ok(())
+    }
+
+    /// Waits for the child to exit completely, returning the status that it
+    /// exited with. See [tokio::process::Child::wait] for in-depth
+    /// documentation.
+    async fn wait(&self) -> io::Result<std::process::ExitStatus> {
+        self.inner.lock().await.wait().await
+    }
+
+    /// Kill the OpenVPN process and drop its stdin handle.
+    async fn stop(&self) {
         // Dropping our stdin handle so that it is closed once. Closing the handle should
         // gracefully stop our OpenVPN child process.
         if self.stdin.lock().take().is_none() {
             log::warn!("Tried to close OpenVPN stdin handle twice, this is a bug");
         }
+        self.clean_up().await
     }
 
-    fn kill(&self) -> io::Result<()> {
+    async fn kill(&self) -> io::Result<()> {
         log::warn!("Killing OpenVPN process");
-        self.inner.kill()?;
+        self.inner.lock().await.kill().await?;
         log::debug!("OpenVPN forcefully killed");
         Ok(())
     }
 
-    fn has_stopped(&self) -> io::Result<bool> {
-        match self.inner.try_wait() {
-            Ok(None) => Ok(false),
-            Ok(Some(_)) => Ok(true),
-            Err(e) => Err(e),
-        }
+    async fn has_stopped(&self) -> io::Result<bool> {
+        let exit_status = self.inner.lock().await.try_wait()?;
+        Ok(exit_status.is_some())
     }
-}
 
-impl Drop for OpenVpnProcHandle {
-    fn drop(&mut self) {
-        let result = match self.has_stopped() {
-            Ok(false) => self.kill(),
+    /// Try to kill the OpenVPN process.
+    async fn clean_up(&self) {
+        let result = match self.has_stopped().await {
+            Ok(false) => self.kill().await,
             Err(e) => {
                 log::error!(
                     "{}",
                     e.display_chain_with_msg("Failed to check if OpenVPN is running")
                 );
-                self.kill()
+                self.kill().await
             }
             _ => Ok(()),
         };
diff --git a/talpid-openvpn/src/process/stoppable_process.rs b/talpid-openvpn/src/process/stoppable_process.rs
deleted file mode 100644
index 3681c6cfa8..0000000000
--- a/talpid-openvpn/src/process/stoppable_process.rs
+++ /dev/null
@@ -1,53 +0,0 @@
-use std::{
-    io, thread,
-    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<()> {
-        log::debug!("Trying to stop child process gracefully");
-        self.stop();
-        if wait_timeout(self, timeout)? {
-            log::debug!("Child process terminated gracefully");
-        } else {
-            log::warn!(
-                "Child process did not terminate gracefully within timeout, forcing termination"
-            );
-            self.kill()?;
-        }
-        Ok(())
-    }
-}
-/// 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)
-}