From db1ffe57d3db14f8227ace38599895e045aa1048 Mon Sep 17 00:00:00 2001 From: Piepmatz Date: Wed, 16 Jul 2025 00:55:02 +0200 Subject: [PATCH] Panic when converting `other` I/O errors into our I/O errors (#16160) # Description I added a debug-only check that makes sure only non-other I/O errors get converted. Since tests run in debug mode, that should help us catch these errors early. I left the check out in release mode on purpose so we don't crash users who have nu as their main shell. It's similar to the debug assertions in the same file that check for unknown spans. --- crates/nu-protocol/src/engine/jobs.rs | 14 +++--- .../nu-protocol/src/errors/shell_error/io.rs | 44 +++++++++++++++++++ crates/nu-system/src/util.rs | 18 +++++--- 3 files changed, 64 insertions(+), 12 deletions(-) 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