diff --git a/crates/nu-protocol/src/engine/jobs.rs b/crates/nu-protocol/src/engine/jobs.rs index 48150ae899..b183f6268f 100644 --- a/crates/nu-protocol/src/engine/jobs.rs +++ b/crates/nu-protocol/src/engine/jobs.rs @@ -11,7 +11,7 @@ use std::time::{Duration, Instant}; use nu_system::{UnfreezeHandle, kill_by_pid}; -use crate::{PipelineData, Signals}; +use crate::{PipelineData, Signals, shell_error}; use crate::JobId; @@ -92,7 +92,7 @@ impl Jobs { /// This function tries to forcefully kill a job from this job table, /// removes it from the job table. It always succeeds in removing the job /// from the table, but may fail in killing the job's active processes. - pub fn kill_and_remove(&mut self, id: JobId) -> std::io::Result<()> { + pub fn kill_and_remove(&mut self, id: JobId) -> shell_error::io::Result<()> { if let Some(job) = self.jobs.get(&id) { let err = job.kill(); @@ -109,7 +109,7 @@ impl Jobs { /// /// It returns an error if any of the job killing attempts fails, but always /// succeeds in removing the jobs from the table. - pub fn kill_all(&mut self) -> std::io::Result<()> { + pub fn kill_all(&mut self) -> shell_error::io::Result<()> { self.last_frozen_job_id = None; let first_err = self @@ -180,7 +180,7 @@ impl ThreadJob { lock.iter().copied().collect() } - pub fn kill(&self) -> std::io::Result<()> { + pub fn kill(&self) -> shell_error::io::Result<()> { // it's okay to make this interrupt outside of the mutex, since it has acquire-release // semantics. @@ -205,7 +205,7 @@ impl ThreadJob { } impl Job { - pub fn kill(&self) -> std::io::Result<()> { + pub fn kill(&self) -> shell_error::io::Result<()> { match self { Job::Thread(thread_job) => thread_job.kill(), Job::Frozen(frozen_job) => frozen_job.kill(), @@ -233,10 +233,10 @@ pub struct FrozenJob { } impl FrozenJob { - pub fn kill(&self) -> std::io::Result<()> { + pub fn kill(&self) -> shell_error::io::Result<()> { #[cfg(unix)] { - kill_by_pid(self.unfreeze.pid() as i64) + Ok(kill_by_pid(self.unfreeze.pid() as i64)?) } // it doesn't happen outside unix. diff --git a/crates/nu-protocol/src/errors/shell_error/io.rs b/crates/nu-protocol/src/errors/shell_error/io.rs index a60a7fae0b..caab488185 100644 --- a/crates/nu-protocol/src/errors/shell_error/io.rs +++ b/crates/nu-protocol/src/errors/shell_error/io.rs @@ -10,6 +10,16 @@ use crate::Span; use super::location::Location; +/// Alias for a `Result` with the error type [`ErrorKind`] by default. +/// +/// This may be used in all situations that would usually return an [`std::io::Error`] but are +/// already part of the [`nu_protocol`](crate) crate and can therefore interact with +/// [`shell_error::io`](self) directly. +/// +/// To make programming inside this module easier, you can pass the `E` type with another error. +/// This avoids the annoyance of having a shadowed `Result`. +pub type Result = std::result::Result; + /// Represents an I/O error in the [`ShellError::Io`] variant. /// /// This is the central I/O error for the [`ShellError::Io`] variant. @@ -158,6 +168,14 @@ pub enum ErrorKind { #[allow(private_interfaces)] Std(std::io::ErrorKind, Sealed), + /// Killing a job process failed. + /// + /// This error is part [`ShellError::Io`](super::ShellError::Io) instead of + /// [`ShellError::Job`](super::ShellError::Job) as this error occurs because some I/O operation + /// failed on the OS side. + /// And not part of our logic. + KillJobProcess, + NotAFile, /// The file or directory is in use by another program. @@ -375,10 +393,34 @@ impl From<&std::io::Error> for ErrorKind { } } + #[cfg(debug_assertions)] + if err.kind() == std::io::ErrorKind::Other { + panic!( + "\ +suspicious conversion: + tried to convert `std::io::Error` with `std::io::ErrorKind::Other` + into `nu_protocol::shell_error::io::ErrorKind` + +I/O errors should always be specific, provide more context + +{err:#?}\ + " + ) + } + ErrorKind::Std(err.kind(), Sealed) } } +impl From for ErrorKind { + fn from(value: nu_system::KillByPidError) -> Self { + match value { + nu_system::KillByPidError::Output(error) => error.into(), + nu_system::KillByPidError::KillProcess => ErrorKind::KillJobProcess, + } + } +} + impl StdError for IoError {} impl Display for IoError { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { @@ -400,6 +442,7 @@ impl Display for ErrorKind { let (first, rest) = msg.split_at(1); write!(f, "{}{}", first.to_uppercase(), rest) } + ErrorKind::KillJobProcess => write!(f, "Killing job process failed"), ErrorKind::NotAFile => write!(f, "Not a file"), ErrorKind::AlreadyInUse => write!(f, "Already in use"), ErrorKind::FileNotFound => write!(f, "File not found"), @@ -437,6 +480,7 @@ impl Diagnostic for IoError { std::io::ErrorKind::Other => code.push_str("other"), kind => code.push_str(&kind.to_string().to_lowercase().replace(" ", "_")), }, + ErrorKind::KillJobProcess => code.push_str("kill_job_process"), ErrorKind::NotAFile => code.push_str("not_a_file"), ErrorKind::AlreadyInUse => code.push_str("already_in_use"), ErrorKind::FileNotFound => code.push_str("file_not_found"), diff --git a/crates/nu-system/src/util.rs b/crates/nu-system/src/util.rs index 5bf20c8827..cc234d4231 100644 --- a/crates/nu-system/src/util.rs +++ b/crates/nu-system/src/util.rs @@ -2,16 +2,24 @@ use std::io; use std::process::Command as CommandSys; /// Tries to forcefully kill a process by its PID -pub fn kill_by_pid(pid: i64) -> io::Result<()> { +pub fn kill_by_pid(pid: i64) -> Result<(), KillByPidError> { let mut cmd = build_kill_command(true, std::iter::once(pid), None); - let output = cmd.output()?; + let output = cmd.output().map_err(KillByPidError::Output)?; - if !output.status.success() { - return Err(io::Error::other("failed to kill process")); + match output.status.success() { + true => Ok(()), + false => Err(KillByPidError::KillProcess), } +} - Ok(()) +/// Error while killing a process forcefully by its PID. +pub enum KillByPidError { + /// I/O error while capturing the output of the process. + Output(io::Error), + + /// Killing the process failed. + KillProcess, } /// Create a `std::process::Command` for the current target platform, for killing