fix child kill

Signed-off-by: Andrei Gherghescu <[email protected]>

Author: andrei-ng

plotly_static/src/webdriver.rs

@@ -10,7 +10,7 @@
 use std::io::prelude::*;
 use std::io::BufReader;
 use std::path::PathBuf;
-use std::process::{Command, Stdio};
+use std::process::{Child, Command, Stdio};
 use std::sync::{Arc, Mutex};
 use std::thread;
 #[cfg(test)]
@@ -39,7 +39,7 @@ pub(crate) const WEBDRIVER_URL: &str = "http://localhost";
 struct WdInner {
     webdriver_port: u32,
     driver_path: PathBuf,
-    webdriver_process_id: Option<u32>,
+    webdriver_child: Option<Child>,
     is_external: bool, /* Marker for whether this WebDriver was spawned by us or connected to
                         * existing */
 }
@@ -97,7 +97,7 @@ impl WebDriver {
             inner: Arc::new(Mutex::new(WdInner {
                 webdriver_port: port,
                 driver_path: full_path,
-                webdriver_process_id: None,
+                webdriver_child: None,
                 is_external: false, // mark it as spawned by us
             })),
         })
@@ -113,51 +113,40 @@ impl WebDriver {
     /// using the `stop()` method.
     pub(crate) fn spawn_webdriver(&mut self) {
         info!("Spawning {WEBDRIVER_BIN}");
-        let local_self = self.inner.clone();
+        
+        // Spawn the process in the main thread to get the Child handle
+        let mut command = Command::new(self.inner.lock().unwrap().driver_path.clone());
+        command
+            .arg(format!("--port={}", self.inner.lock().unwrap().webdriver_port))
+            .stdin(Stdio::piped())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped());
 
-        let _ = thread::spawn(move || {
-            let mut inner = match local_self.lock() {
-                Ok(inner) => inner,
-                Err(e) => {
-                    error!("Could not acquire lock: {e}");
-                    return;
-                }
-            };
-
-            let mut command = Command::new(inner.driver_path.clone());
-            command
-                .arg(format!("--port={}", inner.webdriver_port))
-                .stdin(Stdio::piped())
-                .stdout(Stdio::piped())
-                .stderr(Stdio::piped());
-
-            let mut child = match command.spawn() {
-                Ok(c) => {
-                    inner.webdriver_process_id = Some(c.id());
-                    c
-                }
-                Err(e) => {
-                    error!("Failed to spawn '{WEBDRIVER_BIN}': {e}");
-                    return;
-                }
-            };
-            drop(inner);
+        let mut child = match command.spawn() {
+            Ok(c) => c,
+            Err(e) => {
+                error!("Failed to spawn '{WEBDRIVER_BIN}': {e}");
+                return;
+            }
+        };
 
-            match child.wait() {
-                Ok(c) => {
-                    info!("Terminated with ExitStatus: {c}");
-                    let stderr = child.stderr.take().unwrap();
-                    let stderr_lines = BufReader::new(stderr).lines();
-                    for line in stderr_lines {
-                        let line = line.unwrap();
+        // Spawn a thread to handle the process stderr output (logging only)
+        if let Some(stderr) = child.stderr.take() {
+            thread::spawn(move || {
+                let stderr_lines = BufReader::new(stderr).lines();
+                for line in stderr_lines {
+                    if let Ok(line) = line {
                         debug!("{line}");
                     }
                 }
-                Err(e) => {
-                    error!("Failed waiting on '{WEBDRIVER_BIN}': {e}");
-                }
-            };
-        });
+            });
+        }
+
+        // Store the Child handle (without stderr)
+        {
+            let mut inner = self.inner.lock().unwrap();
+            inner.webdriver_child = Some(child);
+        }
     }
 
     /// Stops the WebDriver process
@@ -172,14 +161,28 @@ impl WebDriver {
     /// Returns `Ok(())` on success, or an error if the process termination
     /// fails.
     pub fn stop(&mut self) -> Result<()> {
-        let inner = self.inner.lock().unwrap();
+        let mut inner = self.inner.lock().unwrap();
 
         // Only stop the process if we spawned it (not if it's external)
         if !inner.is_external {
-            if let Some(id) = inner.webdriver_process_id {
-                info!("Stopping '{WEBDRIVER_BIN}' (PID: {id})");
-                let mut kill = Command::new("kill").arg(id.to_string()).spawn()?;
-                kill.wait()?;
+            if let Some(child) = inner.webdriver_child.as_mut() {
+                info!("Stopping '{WEBDRIVER_BIN}' (PID: {})", child.id());
+                
+                // Use the robust Child::kill() method which works cross-platform
+                match child.kill() {
+                    Ok(_) => {
+                        info!("Successfully sent kill signal to WebDriver process");
+                        // Wait for the process to terminate
+                        match child.wait() {
+                            Ok(status) => info!("WebDriver process terminated with status: {status}"),
+                            Err(e) => warn!("Failed to wait for WebDriver process termination: {e}"),
+                        }
+                    }
+                    Err(e) => {
+                        error!("Failed to kill WebDriver process: {e}");
+                        return Err(anyhow!("Failed to kill WebDriver process: {e}"));
+                    }
+                }
             }
         } else {
             warn!(
@@ -272,7 +275,14 @@ impl WebDriver {
                 None => {
                     // For external connections, we don't need the actual path
                     // Just use a placeholder since we're not spawning
-                    String::from("/usr/bin/webdriver")
+                    #[cfg(target_os = "windows")]
+                    {
+                        String::from("C:\\Windows\\System32\\webdriver.exe")
+                    }
+                    #[cfg(not(target_os = "windows"))]
+                    {
+                        String::from("/usr/bin/webdriver")
+                    }
                 }
             },
         };
@@ -283,7 +293,7 @@ impl WebDriver {
             inner: Arc::new(Mutex::new(WdInner {
                 webdriver_port: port,
                 driver_path: full_path,
-                webdriver_process_id: None,
+                webdriver_child: None,
                 is_external: true, // Mark as external since we didn't spawn it
             })),
         })
@@ -314,8 +324,16 @@ impl WebDriver {
     }
 
     #[cfg(target_os = "windows")]
-    fn os_binary_path(path: PathBuf) -> PathBuf {
+    fn os_binary_path(path: PathBuf) -> Result<PathBuf> {
         let app = format!("{WEBDRIVER_BIN}.exe");
-        path.join(app)
+        let full_path = path.join(app);
+        if full_path.exists() {
+            Ok(full_path)
+        } else {
+            Err(anyhow!(
+                "No {WEBDRIVER_BIN}.exe found at '{}'",
+                path.display()
+            ))
+        }
     }
 }