Bug 1443853 - Rename RunnerProcess::status() to ::try_wait(). r?jgraham draft
authorAndreas Tolfsen <ato@sny.no>
Wed, 07 Mar 2018 21:43:49 +0000
changeset 765980 a57f837800f6de114b429524e02a102f22fa2c29
parent 765979 b162adc88b152f365d66be40377bb126c1baeda9
child 765981 78fff5aa8b635161b2e339c75b26f1d896282010
push id102198
push userbmo:ato@sny.no
push dateSun, 11 Mar 2018 16:03:30 +0000
reviewersjgraham
bugs1443853
milestone60.0a1
Bug 1443853 - Rename RunnerProcess::status() to ::try_wait(). r?jgraham This renames RunnerProcess::status() to ::try_wait() for symmetry with std::process::Child::try_wait() in the standard library. The patch also makes an attempt at cleaning up its usage in geckodriver, however this can be further improved. MozReview-Commit-ID: 14ihT7MpM7l
testing/geckodriver/src/marionette.rs
testing/mozbase/rust/mozrunner/src/runner.rs
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -1336,64 +1336,69 @@ pub struct MarionetteConnection {
     pub session: MarionetteSession
 }
 
 impl MarionetteConnection {
     pub fn new(port: u16, session_id: Option<String>) -> MarionetteConnection {
         MarionetteConnection {
             port: port,
             stream: None,
-            session: MarionetteSession::new(session_id)
+            session: MarionetteSession::new(session_id),
         }
     }
 
     pub fn connect(&mut self, browser: &mut Option<FirefoxProcess>) -> WebDriverResult<()> {
-        let timeout = 60 * 1000;  // ms
-        let poll_interval = 100;  // ms
+        let timeout = 60 * 1000; // ms
+        let poll_interval = 100; // ms
         let poll_attempts = timeout / poll_interval;
         let mut poll_attempt = 0;
 
         loop {
-            // If the process is gone, immediately abort the connection attempts
+            // immediately abort connection attempts if process disappears
             if let &mut Some(ref mut runner) = browser {
-                let status = runner.status();
-                if status.is_err() || status.as_ref().map(|x| *x).unwrap_or(None) != None {
+                let exit_status = match runner.try_wait() {
+                    Ok(Some(status)) => Some(
+                        status
+                            .code()
+                            .map(|c| c.to_string())
+                            .unwrap_or("signal".into()),
+                    ),
+                    Ok(None) => None,
+                    Err(_) => Some("{unknown}".into()),
+                };
+                if let Some(s) = exit_status {
                     return Err(WebDriverError::new(
                         ErrorStatus::UnknownError,
-                        format!("Process unexpectedly closed with status: {}", status
-                            .ok()
-                            .and_then(|x| x)
-                            .and_then(|x| x.code())
-                            .map(|x| x.to_string())
-                            .unwrap_or("{unknown}".into()))));
+                        format!("Process unexpectedly closed with status {}", s),
+                    ));
                 }
             }
 
             match TcpStream::connect(&(DEFAULT_HOST, self.port)) {
                 Ok(stream) => {
                     self.stream = Some(stream);
-                    break
-                },
+                    break;
+                }
                 Err(e) => {
                     trace!("  connection attempt {}/{}", poll_attempt, poll_attempts);
                     if poll_attempt <= poll_attempts {
                         poll_attempt += 1;
                         sleep(Duration::from_millis(poll_interval));
                     } else {
                         return Err(WebDriverError::new(
-                            ErrorStatus::UnknownError, e.description().to_owned()));
+                            ErrorStatus::UnknownError,
+                            e.description().to_owned(),
+                        ));
                     }
                 }
             }
-        };
+        }
 
         debug!("Connected to Marionette on {}:{}", DEFAULT_HOST, self.port);
-
-        try!(self.handshake());
-        Ok(())
+        self.handshake()
     }
 
     fn handshake(&mut self) -> WebDriverResult<()> {
         let resp = try!(self.read_resp());
         let handshake_data = try!(Json::from_str(&*resp));
 
         let data = try_opt!(handshake_data.as_object(),
                             ErrorStatus::UnknownError,
--- a/testing/mozbase/rust/mozrunner/src/runner.rs
+++ b/testing/mozbase/rust/mozrunner/src/runner.rs
@@ -42,17 +42,27 @@ pub trait Runner {
     fn stderr<'a, T>(&'a mut self, stderr: T) -> &'a mut Self
     where
         T: Into<Stdio>;
 
     fn start(self) -> Result<Self::Process, RunnerError>;
 }
 
 pub trait RunnerProcess {
-    fn status(&mut self) -> io::Result<Option<process::ExitStatus>>;
+    /// Attempts to collect the exit status of the process if it has already exited.
+    ///
+    /// This function will not block the calling thread and will only advisorily check to see if
+    /// the child process has exited or not.  If the process has exited then on Unix the process ID
+    /// is reaped.  This function is guaranteed to repeatedly return a successful exit status so
+    /// long as the child has already exited.
+    ///
+    /// If the process has exited, then `Ok(Some(status))` is returned.  If the exit status is not
+    /// available at this time then `Ok(None)` is returned.  If an error occurs, then that error is
+    /// returned.
+    fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>>;
 
     /// Determine if the process is still running.
     fn running(&mut self) -> bool;
 
     /// Forces the process to exit and returns the exit status.  This is
     /// equivalent to sending a SIGKILL on Unix platforms.
     fn kill(&mut self) -> io::Result<process::ExitStatus>;
 }
@@ -104,22 +114,22 @@ impl From<PrefReaderError> for RunnerErr
 
 #[derive(Debug)]
 pub struct FirefoxProcess {
     process: Child,
     profile: Profile,
 }
 
 impl RunnerProcess for FirefoxProcess {
-    fn status(&mut self) -> io::Result<Option<process::ExitStatus>> {
+    fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>> {
         self.process.try_wait()
     }
 
     fn running(&mut self) -> bool {
-        self.status().unwrap().is_none()
+        self.try_wait().unwrap().is_none()
     }
 
     fn kill(&mut self) -> io::Result<process::ExitStatus> {
         self.process.kill()?;
         self.process.wait()
     }
 }