Merge pull request #39 from rust-lang/thiserror

Migrate the command module away from failure

Author: pietroalbini

CHANGELOG.md

@@ -5,10 +5,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Added
+
+- New variant `CommandError::ExecutionFailed`
+- New variant `CommandError::KillAfterTimeoutFailed`
+- New variant `CommandError::SandboxImagePullFailed`
+- New variant `CommandError::SandboxImageMissing`
+- New variant `CommandError::WorkspaceNotMountedCorrectly`
+- New variant `CommandError::InvalidDockerInspectOutput`
+- New variant `CommandError::IO`
+- New struct `KillFailedError`
+
 ### Changed
 
 - **BREAKING**: support for CI toolchains is now gated behind the
   `unstable-toolchain-ci` Cargo feature.
+- **BREAKING**: all functions and methods inside `cmd` now return `CommandError`.
 - `winapi` is no longer required on unix; `nix` is no longer required on windows.
 - Relaxed lifetime restrictions of `Build::cmd` and `Build::cargo`.
 

Cargo.toml

@@ -37,6 +37,7 @@ fs2 = "0.4.3"
 remove_dir_all = "0.5.2"
 base64 = "0.12.0"
 getrandom = { version = "0.1.12", features = ["std"] }
+thiserror = "1.0.20"
 
 [target.'cfg(unix)'.dependencies]
 nix = "0.11.0"

src/cmd/mod.rs

@@ -8,7 +8,6 @@ pub use sandbox::*;
 
 use crate::native;
 use crate::workspace::Workspace;
-use failure::{Error, Fail};
 use futures_util::{
     future::{self, FutureExt},
     stream::{self, TryStreamExt},
@@ -58,20 +57,79 @@ pub(crate) mod container_dirs {
 }
 
 /// Error happened while executing a command.
-#[derive(Debug, Fail)]
+#[derive(Debug, thiserror::Error)]
 #[non_exhaustive]
 pub enum CommandError {
     /// The command didn't output anything to stdout or stderr for more than the timeout, and it
     /// was killed. The timeout's value (in seconds) is the first value.
-    #[fail(display = "no output for {} seconds", _0)]
+    #[error("no output for {0} seconds")]
     NoOutputFor(u64),
+
     /// The command took more time than the timeout to end, and it was killed. The timeout's value
     /// (in seconds) is the first value.
-    #[fail(display = "command timed out after {} seconds", _0)]
+    #[error("command timed out after {0} seconds")]
     Timeout(u64),
+
+    /// The command failed to execute.
+    #[error("command failed: {0}")]
+    ExecutionFailed(ExitStatus),
+
+    /// Killing the underlying process after the timeout failed.
+    #[error("{0}")]
+    KillAfterTimeoutFailed(#[source] KillFailedError),
+
     /// The sandbox ran out of memory and was killed.
-    #[fail(display = "container ran out of memory")]
+    #[error("container ran out of memory")]
     SandboxOOM,
+
+    /// Pulling a sandbox image from the registry failed
+    #[error("failed to pull the sandbox image from the registry: {0}")]
+    SandboxImagePullFailed(#[source] Box<CommandError>),
+
+    /// The sandbox image is missing from the local system.
+    #[error("sandbox image missing from the local system: {0}")]
+    SandboxImageMissing(#[source] Box<CommandError>),
+
+    /// Running rustwide inside a Docker container requires the workspace directory to be mounted
+    /// from the host system. This error happens if that's not true, for example if the workspace
+    /// lives in a directory inside the container.
+    #[error("the workspace is not mounted from outside the container")]
+    WorkspaceNotMountedCorrectly,
+
+    /// The data received from the `docker inspect` command is not valid.
+    #[error("invalid output of `docker inspect`: {0}")]
+    InvalidDockerInspectOutput(#[source] serde_json::Error),
+
+    /// An I/O error occured while executing the command.
+    #[error(transparent)]
+    IO(#[from] std::io::Error),
+}
+
+/// Error happened while trying to kill a process.
+#[derive(Debug, thiserror::Error)]
+#[cfg_attr(unix, error(
+    "failed to kill the process with PID {pid}{}",
+    .errno.map(|e| format!(": {}", e.desc())).unwrap_or_else(String::new)
+))]
+#[cfg_attr(not(unix), error("failed to kill the process with PID {pid}"))]
+pub struct KillFailedError {
+    pub(crate) pid: u32,
+    #[cfg(unix)]
+    pub(crate) errno: Option<nix::errno::Errno>,
+}
+
+impl KillFailedError {
+    /// Return the PID of the process that couldn't be killed.
+    pub fn pid(&self) -> u32 {
+        self.pid
+    }
+
+    /// Return the underlying error number provided by the operative system.
+    #[cfg(any(unix, doc))]
+    #[cfg_attr(docs_rs, doc(cfg(unix)))]
+    pub fn errno(&self) -> Option<i32> {
+        self.errno.map(|errno| errno as i32)
+    }
 }
 
 /// Name and kind of a binary executed by [`Command`](struct.Command.html).
@@ -285,7 +343,7 @@ impl<'w, 'pl> Command<'w, 'pl> {
 
     /// Run the prepared command and return an error if it fails (for example with a non-zero exit
     /// code or a timeout).
-    pub fn run(self) -> Result<(), Error> {
+    pub fn run(self) -> Result<(), CommandError> {
         self.run_inner(false)?;
         Ok(())
     }
@@ -296,11 +354,11 @@ impl<'w, 'pl> Command<'w, 'pl> {
     /// Even though the output will be captured and returned, if output logging is enabled (as it
     /// is by default) the output will be also logged. You can disable this behavior by calling the
     /// [`log_output`](struct.Command.html#method.log_output) method.
-    pub fn run_capture(self) -> Result<ProcessOutput, Error> {
+    pub fn run_capture(self) -> Result<ProcessOutput, CommandError> {
         Ok(self.run_inner(true)?)
     }
 
-    fn run_inner(self, capture: bool) -> Result<ProcessOutput, Error> {
+    fn run_inner(self, capture: bool) -> Result<ProcessOutput, CommandError> {
         if let Some(mut builder) = self.sandbox {
             let workspace = self
                 .workspace
@@ -438,7 +496,7 @@ impl<'w, 'pl> Command<'w, 'pl> {
             if out.status.success() {
                 Ok(out.into())
             } else {
-                failure::bail!("command `{}` failed", cmdstr);
+                Err(CommandError::ExecutionFailed(out.status))
             }
         }
     }
@@ -499,7 +557,7 @@ async fn log_command(
     timeout: Option<Duration>,
     no_output_timeout: Option<Duration>,
     log_output: bool,
-) -> Result<InnerProcessOutput, Error> {
+) -> Result<InnerProcessOutput, CommandError> {
     let timeout = if let Some(t) = timeout {
         t
     } else {
@@ -532,12 +590,12 @@ async fn log_command(
         .map(move |result| match result {
             // If the timeout elapses, kill the process
             Err(_timeout) => Err(match native::kill_process(child_id) {
-                Ok(()) => Error::from(CommandError::NoOutputFor(no_output_timeout.as_secs())),
-                Err(err) => err,
+                Ok(()) => CommandError::NoOutputFor(no_output_timeout.as_secs()),
+                Err(err) => CommandError::KillAfterTimeoutFailed(err),
             }),
 
             // If an error occurred reading the line, flatten the error
-            Ok((_, Err(read_err))) => Err(Error::from(read_err)),
+            Ok((_, Err(read_err))) => Err(read_err.into()),
 
             // If the read was successful, return the `OutputKind` and the read line
             Ok((out_kind, Ok(line))) => Ok((out_kind, line)),
@@ -546,7 +604,7 @@ async fn log_command(
             // If the process is in a tight output loop the timeout on the process might fail to
             // be executed, so this extra check prevents the process to run without limits.
             if start.elapsed() > timeout {
-                return future::err(Error::from(CommandError::Timeout(timeout.as_secs())));
+                return future::err(CommandError::Timeout(timeout.as_secs()));
             }
 
             if let Some(f) = &mut process_lines {
@@ -587,12 +645,12 @@ async fn log_command(
         match result {
             // If the timeout elapses, kill the process
             Err(_timeout) => Err(match native::kill_process(child_id) {
-                Ok(()) => Error::from(CommandError::Timeout(timeout.as_secs())),
-                Err(err) => err,
+                Ok(()) => CommandError::Timeout(timeout.as_secs()),
+                Err(err) => CommandError::KillAfterTimeoutFailed(err),
             }),
 
             // If an error occurred with the child
-            Ok(Err(err)) => Err(Error::from(err)),
+            Ok(Err(err)) => Err(err.into()),
 
             // If the read was successful, return the process's exit status
             Ok(Ok(exit_status)) => Ok(exit_status),

src/cmd/sandbox.rs

@@ -1,8 +1,8 @@
 use crate::cmd::{Command, CommandError, ProcessLinesActions, ProcessOutput};
 use crate::Workspace;
-use failure::Error;
 use log::{error, info};
 use serde::Deserialize;
+use std::error::Error;
 use std::fmt;
 use std::path::{Path, PathBuf};
 use std::time::Duration;
@@ -16,7 +16,7 @@ impl SandboxImage {
     /// Load a local image present in the host machine.
     ///
     /// If the image is not available locally an error will be returned instead.
-    pub fn local(name: &str) -> Result<Self, Error> {
+    pub fn local(name: &str) -> Result<Self, CommandError> {
         let image = SandboxImage { name: name.into() };
         info!("sandbox image is local, skipping pull");
         image.ensure_exists_locally()?;
@@ -27,12 +27,13 @@ impl SandboxImage {
     ///
     /// This will access the network to download the image from the registry. If pulling fails an
     /// error will be returned instead.
-    pub fn remote(name: &str) -> Result<Self, Error> {
+    pub fn remote(name: &str) -> Result<Self, CommandError> {
         let mut image = SandboxImage { name: name.into() };
         info!("pulling image {} from Docker Hub", name);
         Command::new_workspaceless("docker")
             .args(&["pull", &name])
-            .run()?;
+            .run()
+            .map_err(|e| CommandError::SandboxImagePullFailed(Box::new(e)))?;
         if let Some(name_with_hash) = image.get_name_with_hash() {
             image.name = name_with_hash;
             info!("pulled image {}", image.name);
@@ -41,12 +42,13 @@ impl SandboxImage {
         Ok(image)
     }
 
-    fn ensure_exists_locally(&self) -> Result<(), Error> {
+    fn ensure_exists_locally(&self) -> Result<(), CommandError> {
         info!("checking the image {} is available locally", self.name);
         Command::new_workspaceless("docker")
             .args(&["image", "inspect", &self.name])
             .log_output(false)
-            .run()?;
+            .run()
+            .map_err(|e| CommandError::SandboxImageMissing(Box::new(e)))?;
         Ok(())
     }
 
@@ -85,7 +87,7 @@ struct MountConfig {
 }
 
 impl MountConfig {
-    fn host_path(&self, workspace: &Workspace) -> Result<PathBuf, Error> {
+    fn host_path(&self, workspace: &Workspace) -> Result<PathBuf, CommandError> {
         if let Some(container) = workspace.current_container() {
             // If we're inside a Docker container we'll need to remap the mount sources to point to
             // the directories in the host system instead of the containers. To do that we try to
@@ -97,13 +99,13 @@ impl MountConfig {
                     return Ok(Path::new(mount.source()).join(shared));
                 }
             }
-            failure::bail!("the workspace is not mounted from outside the container");
+            Err(CommandError::WorkspaceNotMountedCorrectly)
         } else {
             Ok(crate::utils::normalize_path(&self.host_path))
         }
     }
 
-    fn to_volume_arg(&self, workspace: &Workspace) -> Result<String, Error> {
+    fn to_volume_arg(&self, workspace: &Workspace) -> Result<String, CommandError> {
         let perm = match self.perm {
             MountKind::ReadWrite => "rw",
             MountKind::ReadOnly => "ro",
@@ -116,7 +118,7 @@ impl MountConfig {
         ))
     }
 
-    fn to_mount_arg(&self, workspace: &Workspace) -> Result<String, Error> {
+    fn to_mount_arg(&self, workspace: &Workspace) -> Result<String, CommandError> {
         let mut opts_with_leading_comma = vec![];
 
         if self.perm == MountKind::ReadOnly {
@@ -214,7 +216,7 @@ impl SandboxBuilder {
         self
     }
 
-    fn create(self, workspace: &Workspace) -> Result<Container<'_>, Error> {
+    fn create(self, workspace: &Workspace) -> Result<Container<'_>, CommandError> {
         let mut args: Vec<String> = vec!["create".into()];
 
         for mount in &self.mounts {
@@ -285,16 +287,18 @@ impl SandboxBuilder {
         log_output: bool,
         log_command: bool,
         capture: bool,
-    ) -> Result<ProcessOutput, Error> {
+    ) -> Result<ProcessOutput, CommandError> {
         let container = self.create(workspace)?;
 
         // Ensure the container is properly deleted even if something panics
         scopeguard::defer! {{
             if let Err(err) = container.delete() {
                 error!("failed to delete container {}", container.id);
                 error!("caused by: {}", err);
-                for cause in err.iter_causes() {
+                let mut err: &dyn Error = &err;
+                while let Some(cause) = err.source() {
                     error!("caused by: {}", cause);
+                    err = cause;
                 }
             }
         }}
@@ -336,14 +340,15 @@ impl fmt::Display for Container<'_> {
 }
 
 impl Container<'_> {
-    fn inspect(&self) -> Result<InspectContainer, Error> {
+    fn inspect(&self) -> Result<InspectContainer, CommandError> {
         let output = Command::new(self.workspace, "docker")
             .args(&["inspect", &self.id])
             .log_output(false)
             .run_capture()?;
 
         let mut data: Vec<InspectContainer> =
-            ::serde_json::from_str(&output.stdout_lines().join("\n"))?;
+            ::serde_json::from_str(&output.stdout_lines().join("\n"))
+                .map_err(CommandError::InvalidDockerInspectOutput)?;
         assert_eq!(data.len(), 1);
         Ok(data.pop().unwrap())
     }
@@ -356,7 +361,7 @@ impl Container<'_> {
         log_output: bool,
         log_command: bool,
         capture: bool,
-    ) -> Result<ProcessOutput, Error> {
+    ) -> Result<ProcessOutput, CommandError> {
         let mut cmd = Command::new(self.workspace, "docker")
             .args(&["start", "-a", &self.id])
             .timeout(timeout)
@@ -373,17 +378,16 @@ impl Container<'_> {
 
         // Return a different error if the container was killed due to an OOM
         if details.state.oom_killed {
-            if let Err(err) = res {
-                Err(err.context(CommandError::SandboxOOM).into())
-            } else {
-                Err(CommandError::SandboxOOM.into())
-            }
+            Err(match res {
+                Ok(_) | Err(CommandError::ExecutionFailed(_)) => CommandError::SandboxOOM,
+                Err(err) => err,
+            })
         } else {
             res
         }
     }
 
-    fn delete(&self) -> Result<(), Error> {
+    fn delete(&self) -> Result<(), CommandError> {
         Command::new(self.workspace, "docker")
             .args(&["rm", "-f", &self.id])
             .run()

src/native/unix.rs

@@ -1,3 +1,4 @@
+use crate::cmd::KillFailedError;
 use failure::Error;
 use nix::{
     sys::signal::{kill, Signal},
@@ -9,9 +10,18 @@ use std::path::Path;
 
 const EXECUTABLE_BITS: u32 = 0o5;
 
-pub(crate) fn kill_process(id: u32) -> Result<(), Error> {
-    kill(Pid::from_raw(id as i32), Signal::SIGKILL)?;
-    Ok(())
+pub(crate) fn kill_process(id: u32) -> Result<(), KillFailedError> {
+    match kill(Pid::from_raw(id as i32), Signal::SIGKILL) {
+        Ok(()) => Ok(()),
+        Err(err) => Err(KillFailedError {
+            pid: id,
+            errno: if let nix::Error::Sys(errno) = err {
+                Some(errno)
+            } else {
+                None
+            },
+        }),
+    }
 }
 
 pub(crate) fn current_user() -> Option<u32> {