From d87e79797ffaaced2bc89477a1c0d78ad74a365b Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 11:49:34 -0800 Subject: [PATCH 01/18] Improve signal handling --- Cargo.lock | 1 + Cargo.toml | 1 + src/command_ext.rs | 18 ++++++- src/error.rs | 8 +++ src/evaluator.rs | 54 +++++++++++++------ src/interrupt_guard.rs | 16 ------ src/interrupt_handler.rs | 86 ----------------------------- src/justfile.rs | 22 ++++---- src/lib.rs | 36 +++++++------ src/output.rs | 34 ------------ src/output_error.rs | 19 +++++++ src/platform.rs | 16 ++++++ src/platform_interface.rs | 18 ++++--- src/recipe.rs | 22 ++++++-- src/run.rs | 2 +- src/signal.rs | 83 ++++++++++++++++++++++++++++ src/signal_handler.rs | 110 ++++++++++++++++++++++++++++++++++++++ src/signals.rs | 90 +++++++++++++++++++++++++++++++ tests/format.rs | 2 +- tests/interrupts.rs | 13 +++-- tests/lib.rs | 2 + tests/signals.rs | 34 ++++++++++++ 22 files changed, 487 insertions(+), 200 deletions(-) delete mode 100644 src/interrupt_guard.rs delete mode 100644 src/interrupt_handler.rs delete mode 100644 src/output.rs create mode 100644 src/signal.rs create mode 100644 src/signal_handler.rs create mode 100644 src/signals.rs create mode 100644 tests/signals.rs diff --git a/Cargo.lock b/Cargo.lock index 38bbaec345..e7784dca92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -539,6 +539,7 @@ dependencies = [ "heck", "lexiclean", "libc", + "nix", "num_cpus", "once_cell", "percent-encoding", diff --git a/Cargo.toml b/Cargo.toml index f10743b9af..9b0bce307e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ edit-distance = "2.0.0" heck = "0.5.0" lexiclean = "0.0.1" libc = "0.2.0" +nix = { version = "0.29.0", features = ["user"] } num_cpus = "1.15.0" once_cell = "1.19.0" percent-encoding = "2.3.1" diff --git a/src/command_ext.rs b/src/command_ext.rs index 40d9da1600..0275227d24 100644 --- a/src/command_ext.rs +++ b/src/command_ext.rs @@ -7,9 +7,13 @@ pub(crate) trait CommandExt { dotenv: &BTreeMap, scope: &Scope, unexports: &HashSet, - ); + ) -> &mut Command; fn export_scope(&mut self, settings: &Settings, scope: &Scope, unexports: &HashSet); + + fn status_guard(self) -> (io::Result, Option); + + fn output_guard(self) -> (io::Result, Option); } impl CommandExt for Command { @@ -19,7 +23,7 @@ impl CommandExt for Command { dotenv: &BTreeMap, scope: &Scope, unexports: &HashSet, - ) { + ) -> &mut Command { for (name, value) in dotenv { self.env(name, value); } @@ -27,6 +31,8 @@ impl CommandExt for Command { if let Some(parent) = scope.parent() { self.export_scope(settings, parent, unexports); } + + self } fn export_scope(&mut self, settings: &Settings, scope: &Scope, unexports: &HashSet) { @@ -44,4 +50,12 @@ impl CommandExt for Command { } } } + + fn status_guard(self) -> (io::Result, Option) { + SignalHandler::spawn(self, |mut child| child.wait()) + } + + fn output_guard(self) -> (io::Result, Option) { + SignalHandler::spawn(self, process::Child::wait_with_output) + } } diff --git a/src/error.rs b/src/error.rs index b8774a2515..b6b423be2f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -116,6 +116,9 @@ pub(crate) enum Error<'src> { Internal { message: String, }, + Interrupted { + signal: Signal, + }, Io { recipe: &'src str, io_error: io::Error, @@ -291,6 +294,7 @@ impl ColorDisplay for Error<'_> { OutputError::Code(code) => write!(f, "Backtick failed with exit code {code}")?, OutputError::Signal(signal) => write!(f, "Backtick was terminated by signal {signal}")?, OutputError::Unknown => write!(f, "Backtick failed for an unknown reason")?, + OutputError::Interrupted(signal) => write!(f, "Backtick succeeded but `just` was interrupted by signal {signal}")?, OutputError::Io(io_error) => match io_error.kind() { io::ErrorKind::NotFound => write!(f, "Backtick could not be run because just could not find the shell:\n{io_error}"), io::ErrorKind::PermissionDenied => write!(f, "Backtick could not be run because just could not run the shell:\n{io_error}"), @@ -340,6 +344,7 @@ impl ColorDisplay for Error<'_> { OutputError::Code(code) => write!(f, "Cygpath failed with exit code {code} while translating recipe `{recipe}` shebang interpreter path")?, OutputError::Signal(signal) => write!(f, "Cygpath terminated by signal {signal} while translating recipe `{recipe}` shebang interpreter path")?, OutputError::Unknown => write!(f, "Cygpath experienced an unknown failure while translating recipe `{recipe}` shebang interpreter path")?, + OutputError::Interrupted(signal) => write!(f, "Cygpath succeeded but `just` was interrupted by {signal}")?, OutputError::Io(io_error) => { match io_error.kind() { io::ErrorKind::NotFound => write!(f, "Could not find `cygpath` executable to translate recipe `{recipe}` shebang interpreter path:\n{io_error}"), @@ -402,6 +407,9 @@ impl ColorDisplay for Error<'_> { write!(f, "Internal runtime error, this may indicate a bug in just: {message} \ consider filing an issue: https://github.com/casey/just/issues/new")?; } + Interrupted { signal } => { + write!(f, "Interrupted by {signal}")?; + } Io { recipe, io_error } => { match io_error.kind() { io::ErrorKind::NotFound => write!(f, "Recipe `{recipe}` could not be run because just could not find the shell: {io_error}"), diff --git a/src/evaluator.rs b/src/evaluator.rs index b83c8cf70c..70ce82c0b3 100644 --- a/src/evaluator.rs +++ b/src/evaluator.rs @@ -264,22 +264,44 @@ impl<'src, 'run> Evaluator<'src, 'run> { .module .settings .shell_command(self.context.config); - cmd.arg(command); - cmd.args(args); - cmd.current_dir(self.context.working_directory()); - cmd.export( - &self.context.module.settings, - self.context.dotenv, - &self.scope, - &self.context.module.unexports, - ); - cmd.stdin(Stdio::inherit()); - cmd.stderr(if self.context.config.verbosity.quiet() { - Stdio::null() - } else { - Stdio::inherit() - }); - InterruptHandler::guard(|| output(cmd)) + + cmd + .arg(command) + .args(args) + .current_dir(self.context.working_directory()) + .export( + &self.context.module.settings, + self.context.dotenv, + &self.scope, + &self.context.module.unexports, + ) + .stdin(Stdio::inherit()) + .stderr(if self.context.config.verbosity.quiet() { + Stdio::null() + } else { + Stdio::inherit() + }) + .stdout(Stdio::piped()); + + let (result, caught) = cmd.output_guard(); + + let output = result.map_err(OutputError::Io)?; + + OutputError::result_from_exit_status(output.status)?; + + let output = str::from_utf8(&output.stdout).map_err(OutputError::Utf8)?; + + if let Some(signal) = caught { + return Err(OutputError::Interrupted(signal)); + } + + Ok( + output + .strip_suffix("\r\n") + .or_else(|| output.strip_suffix("\n")) + .unwrap_or(output) + .into(), + ) } pub(crate) fn evaluate_line( diff --git a/src/interrupt_guard.rs b/src/interrupt_guard.rs deleted file mode 100644 index 23afa1dd87..0000000000 --- a/src/interrupt_guard.rs +++ /dev/null @@ -1,16 +0,0 @@ -use super::*; - -pub(crate) struct InterruptGuard; - -impl InterruptGuard { - pub(crate) fn new() -> Self { - InterruptHandler::instance().block(); - Self - } -} - -impl Drop for InterruptGuard { - fn drop(&mut self) { - InterruptHandler::instance().unblock(); - } -} diff --git a/src/interrupt_handler.rs b/src/interrupt_handler.rs deleted file mode 100644 index 1105954bfe..0000000000 --- a/src/interrupt_handler.rs +++ /dev/null @@ -1,86 +0,0 @@ -use super::*; - -pub(crate) struct InterruptHandler { - blocks: u32, - interrupted: bool, - verbosity: Verbosity, -} - -impl InterruptHandler { - pub(crate) fn install(verbosity: Verbosity) -> Result<(), ctrlc::Error> { - let mut instance = Self::instance(); - instance.verbosity = verbosity; - ctrlc::set_handler(|| Self::instance().interrupt()) - } - - pub(crate) fn instance() -> MutexGuard<'static, Self> { - static INSTANCE: Mutex = Mutex::new(InterruptHandler::new()); - - match INSTANCE.lock() { - Ok(guard) => guard, - Err(poison_error) => { - eprintln!( - "{}", - Error::Internal { - message: format!("interrupt handler mutex poisoned: {poison_error}"), - } - .color_display(Color::auto().stderr()) - ); - process::exit(EXIT_FAILURE); - } - } - } - - const fn new() -> Self { - Self { - blocks: 0, - interrupted: false, - verbosity: Verbosity::default(), - } - } - - fn interrupt(&mut self) { - self.interrupted = true; - - if self.blocks > 0 { - return; - } - - Self::exit(); - } - - fn exit() { - process::exit(130); - } - - pub(crate) fn block(&mut self) { - self.blocks += 1; - } - - pub(crate) fn unblock(&mut self) { - if self.blocks == 0 { - if self.verbosity.loud() { - eprintln!( - "{}", - Error::Internal { - message: "attempted to unblock interrupt handler, but handler was not blocked" - .to_owned(), - } - .color_display(Color::auto().stderr()) - ); - } - process::exit(EXIT_FAILURE); - } - - self.blocks -= 1; - - if self.interrupted { - Self::exit(); - } - } - - pub(crate) fn guard T>(function: F) -> T { - let _guard = InterruptGuard::new(); - function() - } -} diff --git a/src/justfile.rs b/src/justfile.rs index 4af175c1ed..7210c8b204 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -110,20 +110,20 @@ impl<'src> Justfile<'src> { Command::new(binary) }; - command.args(arguments); - - command.current_dir(&search.working_directory); + command + .args(arguments) + .current_dir(&search.working_directory); let scope = scope.child(); command.export(&self.settings, &dotenv, &scope, &self.unexports); - let status = InterruptHandler::guard(|| command.status()).map_err(|io_error| { - Error::CommandInvoke { - binary: binary.clone(), - arguments: arguments.clone(), - io_error, - } + let (result, caught) = command.status_guard(); + + let status = result.map_err(|io_error| Error::CommandInvoke { + binary: binary.clone(), + arguments: arguments.clone(), + io_error, })?; if !status.success() { @@ -134,6 +134,10 @@ impl<'src> Justfile<'src> { }); }; + if let Some(signal) = caught { + return Err(Error::Interrupted { signal }); + } + return Ok(()); } Subcommand::Evaluate { variable, .. } => { diff --git a/src/lib.rs b/src/lib.rs index 799cc37832..dc16ed24b8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,22 +15,21 @@ pub(crate) use { constants::constants, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, execution_context::ExecutionContext, executor::Executor, expression::Expression, - fragment::Fragment, function::Function, interpreter::Interpreter, - interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, + fragment::Fragment, function::Function, interpreter::Interpreter, item::Item, justfile::Justfile, keyed::Keyed, keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, loader::Loader, module_path::ModulePath, name::Name, - namepath::Namepath, ordinal::Ordinal, output::output, output_error::OutputError, - parameter::Parameter, parameter_kind::ParameterKind, parser::Parser, platform::Platform, + namepath::Namepath, ordinal::Ordinal, output_error::OutputError, parameter::Parameter, + parameter_kind::ParameterKind, parser::Parser, platform::Platform, platform_interface::PlatformInterface, position::Position, positional::Positional, ran::Ran, range_ext::RangeExt, recipe::Recipe, recipe_resolver::RecipeResolver, recipe_signature::RecipeSignature, scope::Scope, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, settings::Settings, shebang::Shebang, - show_whitespace::ShowWhitespace, source::Source, string_delimiter::StringDelimiter, - string_kind::StringKind, string_literal::StringLiteral, subcommand::Subcommand, - suggestion::Suggestion, table::Table, thunk::Thunk, token::Token, token_kind::TokenKind, - unresolved_dependency::UnresolvedDependency, unresolved_recipe::UnresolvedRecipe, - unstable_feature::UnstableFeature, use_color::UseColor, variables::Variables, - verbosity::Verbosity, warning::Warning, + show_whitespace::ShowWhitespace, signal::Signal, signal_handler::SignalHandler, + signals::Signals, source::Source, string_delimiter::StringDelimiter, string_kind::StringKind, + string_literal::StringLiteral, subcommand::Subcommand, suggestion::Suggestion, table::Table, + thunk::Thunk, token::Token, token_kind::TokenKind, unresolved_dependency::UnresolvedDependency, + unresolved_recipe::UnresolvedRecipe, unstable_feature::UnstableFeature, use_color::UseColor, + variables::Variables, verbosity::Verbosity, warning::Warning, }, camino::Utf8Path, clap::ValueEnum, @@ -52,18 +51,23 @@ pub(crate) use { env, ffi::OsString, fmt::{self, Debug, Display, Formatter}, - fs, + fs::{self, File}, io::{self, Read, Seek, Write}, iter::{self, FromIterator}, mem, ops::Deref, ops::{Index, Range, RangeInclusive}, + os::fd::{BorrowedFd, IntoRawFd, RawFd}, path::{self, Path, PathBuf}, process::{self, Command, ExitStatus, Stdio}, + ptr, rc::Rc, str::{self, Chars}, - sync::{Mutex, MutexGuard, OnceLock}, - vec, + sync::{ + atomic::{self, AtomicBool, AtomicI32}, + Mutex, MutexGuard, OnceLock, + }, + thread, vec, }, strum::{Display, EnumDiscriminants, EnumString, IntoStaticStr}, tempfile::tempfile, @@ -141,8 +145,6 @@ mod expression; mod fragment; mod function; mod interpreter; -mod interrupt_guard; -mod interrupt_handler; mod item; mod justfile; mod keyed; @@ -156,7 +158,6 @@ mod module_path; mod name; mod namepath; mod ordinal; -mod output; mod output_error; mod parameter; mod parameter_kind; @@ -180,6 +181,9 @@ mod setting; mod settings; mod shebang; mod show_whitespace; +mod signal; +mod signal_handler; +mod signals; mod source; mod string_delimiter; mod string_kind; diff --git a/src/output.rs b/src/output.rs deleted file mode 100644 index f9f4ab024c..0000000000 --- a/src/output.rs +++ /dev/null @@ -1,34 +0,0 @@ -use super::*; - -/// Run a command and return the data it wrote to stdout as a string -pub(crate) fn output(mut command: Command) -> Result { - match command.output() { - Ok(output) => { - if let Some(code) = output.status.code() { - if code != 0 { - return Err(OutputError::Code(code)); - } - } else { - let signal = Platform::signal_from_exit_status(output.status); - return Err(match signal { - Some(signal) => OutputError::Signal(signal), - None => OutputError::Unknown, - }); - } - match str::from_utf8(&output.stdout) { - Err(error) => Err(OutputError::Utf8(error)), - Ok(output) => Ok( - if output.ends_with("\r\n") { - &output[0..output.len() - 2] - } else if output.ends_with('\n') { - &output[0..output.len() - 1] - } else { - output - } - .to_owned(), - ), - } - } - Err(io_error) => Err(OutputError::Io(io_error)), - } -} diff --git a/src/output_error.rs b/src/output_error.rs index b1a165583b..fcd06e3167 100644 --- a/src/output_error.rs +++ b/src/output_error.rs @@ -6,6 +6,8 @@ pub(crate) enum OutputError { Code(i32), /// IO error Io(io::Error), + /// Interrupted by signal + Interrupted(Signal), /// Terminated by signal Signal(i32), /// Unknown failure @@ -14,11 +16,28 @@ pub(crate) enum OutputError { Utf8(str::Utf8Error), } +impl OutputError { + pub(crate) fn result_from_exit_status(exit_status: ExitStatus) -> Result<(), OutputError> { + match exit_status.code() { + Some(0) => Ok(()), + Some(code) => Err(Self::Code(code)), + None => match Platform::signal_from_exit_status(exit_status) { + Some(signal) => Err(Self::Signal(signal)), + None => Err(Self::Unknown), + }, + } + } +} + impl Display for OutputError { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match *self { Self::Code(code) => write!(f, "Process exited with status code {code}"), Self::Io(ref io_error) => write!(f, "Error executing process: {io_error}"), + Self::Interrupted(signal) => write!( + f, + "Process succeded but `just` was interrupted by signal {signal}" + ), Self::Signal(signal) => write!(f, "Process terminated by signal {signal}"), Self::Unknown => write!(f, "Process experienced an unknown failure"), Self::Utf8(ref err) => write!(f, "Could not convert process stdout to UTF-8: {err}"), diff --git a/src/platform.rs b/src/platform.rs index ece80fe133..b393a1dfd5 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -44,6 +44,15 @@ impl PlatformInterface for Platform { .map(str::to_string) .ok_or_else(|| String::from("Error getting current directory: unicode decode error")) } + + fn install_signal_handler(handler: T) -> RunResult<'static> { + thread::spawn(move || { + for signal in Signals::new().unwrap() { + handler(signal.unwrap()); + } + }); + Ok(()) + } } #[cfg(windows)] @@ -112,4 +121,11 @@ impl PlatformInterface for Platform { .ok_or_else(|| String::from("Error getting current directory: unicode decode error")), } } + + fn install_signal_handler( + handler: T, + ) -> Result<(), Box> { + ctrlc::set_handler(|| handler(Signal::Interrupt))?; + Ok(()) + } } diff --git a/src/platform_interface.rs b/src/platform_interface.rs index 463f6650a6..9020a5ab54 100644 --- a/src/platform_interface.rs +++ b/src/platform_interface.rs @@ -1,21 +1,23 @@ use super::*; pub(crate) trait PlatformInterface { - /// Construct a command equivalent to running the script at `path` with the - /// shebang line `shebang` + /// translate path from "native" path to a path the interpreter expects + fn convert_native_path(working_directory: &Path, path: &Path) -> FunctionResult; + + /// install handler, may only be called once + fn install_signal_handler(handler: T) -> RunResult<'static>; + + /// construct command equivalent to running script at `path` with shebang + /// line `shebang` fn make_shebang_command( path: &Path, working_directory: Option<&Path>, shebang: Shebang, ) -> Result; - /// Set the execute permission on the file pointed to by `path` + /// set the execute permission on file pointed to by `path` fn set_execute_permission(path: &Path) -> io::Result<()>; - /// Extract the signal from a process exit status, if it was terminated by a - /// signal + /// extract signal from process exit status fn signal_from_exit_status(exit_status: ExitStatus) -> Option; - - /// Translate a path from a "native" path to a path the interpreter expects - fn convert_native_path(working_directory: &Path, path: &Path) -> FunctionResult; } diff --git a/src/recipe.rs b/src/recipe.rs index 81fbc7ac2f..75aa9d8365 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -289,7 +289,9 @@ impl<'src, D> Recipe<'src, D> { &context.module.unexports, ); - match InterruptHandler::guard(|| cmd.status()) { + let (result, caught) = cmd.status_guard(); + + match result { Ok(exit_status) => { if let Some(code) = exit_status.code() { if code != 0 && !infallible_line { @@ -315,6 +317,10 @@ impl<'src, D> Recipe<'src, D> { }); } }; + + if let Some(signal) = caught { + return Err(Error::Interrupted { signal }); + } } } @@ -433,7 +439,9 @@ impl<'src, D> Recipe<'src, D> { ); // run it! - match InterruptHandler::guard(|| command.status()) { + let (result, caught) = command.status_guard(); + + match result { Ok(exit_status) => exit_status.code().map_or_else( || Err(error_from_signal(self.name(), None, exit_status)), |code| { @@ -448,9 +456,15 @@ impl<'src, D> Recipe<'src, D> { }) } }, - ), - Err(io_error) => Err(executor.error(io_error, self.name())), + )?, + Err(io_error) => return Err(executor.error(io_error, self.name())), } + + if let Some(signal) = caught { + return Err(Error::Interrupted { signal }); + } + + Ok(()) } pub(crate) fn groups(&self) -> BTreeSet { diff --git a/src/run.rs b/src/run.rs index a07b73bbc9..53e37fb52b 100644 --- a/src/run.rs +++ b/src/run.rs @@ -24,7 +24,7 @@ pub fn run(args: impl Iterator + Clone>) -> Result<() config .and_then(|config| { - InterruptHandler::install(config.verbosity).ok(); + SignalHandler::install(config.verbosity)?; config.subcommand.execute(&config, &loader) }) .map_err(|error| { diff --git a/src/signal.rs b/src/signal.rs new file mode 100644 index 0000000000..9ffafcefd4 --- /dev/null +++ b/src/signal.rs @@ -0,0 +1,83 @@ +use super::*; + +#[derive(Clone, Copy, Debug, PartialEq)] +#[repr(i32)] +pub(crate) enum Signal { + Hangup = libc::SIGHUP, + Info = libc::SIGINFO, + Interrupt = libc::SIGINT, + Quit = libc::SIGQUIT, + Terminate = libc::SIGTERM, +} + +impl Signal { + pub(crate) const ALL: [Signal; 5] = [ + Signal::Hangup, + Signal::Info, + Signal::Interrupt, + Signal::Quit, + Signal::Terminate, + ]; + + pub(crate) fn number(self) -> libc::c_int { + self as libc::c_int + } +} + +impl Display for Signal { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { + write!( + f, + "{}", + match self { + Signal::Hangup => "SIGHUP", + Signal::Info => "SIGINFO", + Signal::Interrupt => "SIGINT", + Signal::Quit => "SIGQUIT", + Signal::Terminate => "SIGTERM", + } + ) + } +} + +impl From for nix::sys::signal::Signal { + fn from(signal: Signal) -> Self { + match signal { + Signal::Hangup => Self::SIGHUP, + Signal::Info => Self::SIGINFO, + Signal::Interrupt => Self::SIGINT, + Signal::Quit => Self::SIGQUIT, + Signal::Terminate => Self::SIGTERM, + } + } +} + +impl TryFrom for Signal { + type Error = io::Error; + + fn try_from(n: u8) -> Result { + match n.into() { + libc::SIGHUP => Ok(Signal::Hangup), + libc::SIGINFO => Ok(Signal::Info), + libc::SIGINT => Ok(Signal::Interrupt), + libc::SIGQUIT => Ok(Signal::Quit), + libc::SIGTERM => Ok(Signal::Terminate), + _ => Err(io::Error::new( + io::ErrorKind::Other, + format!("unexpected signal: {n}"), + )), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn signals_fit_in_u8() { + for signal in Signal::ALL { + assert!(signal.number() <= i32::from(u8::MAX)); + } + } +} diff --git a/src/signal_handler.rs b/src/signal_handler.rs new file mode 100644 index 0000000000..a054e931a1 --- /dev/null +++ b/src/signal_handler.rs @@ -0,0 +1,110 @@ +use super::*; + +use nix::unistd::Pid; + +pub(crate) struct SignalHandler { + caught: Option, + children: BTreeMap, + verbosity: Verbosity, +} + +impl SignalHandler { + pub(crate) fn install(verbosity: Verbosity) -> RunResult<'static> { + let mut instance = Self::instance(); + instance.verbosity = verbosity; + Platform::install_signal_handler(|signal| Self::instance().interrupt(signal)) + } + + pub(crate) fn instance() -> MutexGuard<'static, Self> { + static INSTANCE: Mutex = Mutex::new(SignalHandler::new()); + + match INSTANCE.lock() { + Ok(guard) => guard, + Err(poison_error) => { + eprintln!( + "{}", + Error::Internal { + message: format!("signal handler mutex poisoned: {poison_error}"), + } + .color_display(Color::auto().stderr()) + ); + process::exit(EXIT_FAILURE); + } + } + } + + const fn new() -> Self { + Self { + caught: None, + children: BTreeMap::new(), + verbosity: Verbosity::default(), + } + } + + fn interrupt(&mut self, signal: Signal) { + if self.children.is_empty() { + process::exit(128 + signal.number()); + } + + if signal != Signal::Info && self.caught.is_none() { + self.caught = Some(signal); + } + + match signal { + // SIGTERM is the default signal sent by kill. forward it to child + // processes and wait for them to exit + Signal::Terminate => { + for &child in self.children.keys() { + eprintln!("sending sigterm to {child}"); + nix::sys::signal::kill(child, Some(Signal::Terminate.into())); + } + } + Signal::Info => { + // todo: print pid + if self.children.is_empty() { + eprintln!("just: no child processes"); + } else { + // todo: handle plural correctly + let mut message = format!("just: {} child processes:\n", self.children.len()); + + for (&child, command) in &self.children { + message.push_str(&format!("{child}: {command:?}\n")); + } + + eprint!("{message}"); + } + } + // SIGHUP, SIGINT, and SIGQUIT are normally sent on terminal close, + // ctrl-c, and ctrl-\, respectively, and are sent to all processes in the + // foreground process group. this includes child processes, so we ignore + // the signal and rely on them to exit + Signal::Hangup | Signal::Interrupt | Signal::Quit => {} + } + } + + pub(crate) fn spawn( + mut command: Command, + f: impl Fn(process::Child) -> io::Result, + ) -> (io::Result, Option) { + let mut instance = Self::instance(); + + let child = match command.spawn() { + Err(err) => return (Err(err), None), + Ok(child) => child, + }; + + let pid = Pid::from_raw(child.id() as i32); + + instance.children.insert(pid, command); + + drop(instance); + + let result = f(child); + + let mut instance = Self::instance(); + + instance.children.remove(&pid); + + (result, instance.caught) + } +} diff --git a/src/signals.rs b/src/signals.rs new file mode 100644 index 0000000000..b5e5de4943 --- /dev/null +++ b/src/signals.rs @@ -0,0 +1,90 @@ +use { + super::*, + nix::{ + errno::Errno, + sys::signal::{SaFlags, SigAction, SigHandler, SigSet}, + }, +}; + +static WRITE: AtomicI32 = AtomicI32::new(0); + +extern "C" fn handler(signal: libc::c_int) { + let last = Errno::last(); + + let fd = WRITE.load(atomic::Ordering::Relaxed); + + let fd = unsafe { BorrowedFd::borrow_raw(fd) }; + + if let Err(err) = nix::unistd::write(fd, &[signal as u8]) { + // there are few times in life when one is well and truly fucked. this is + // one. + } + + last.set(); +} + +extern "C" fn handler_old(signal: libc::c_int) { + let errno = unsafe { *libc::__error() }; + + let buffer = &[signal as u8]; + + let fd = WRITE.load(atomic::Ordering::Relaxed); + + unsafe { + libc::write(fd, buffer.as_ptr().cast(), buffer.len()); + } + + // todo: should we abort if errno is bad? + + unsafe { + *libc::__error() = errno; + } +} + +pub(crate) struct Signals(File); + +impl Signals { + pub(crate) fn new() -> io::Result { + let (read, write) = nix::unistd::pipe()?; + + if WRITE + .compare_exchange( + 0, + write.into_raw_fd(), + atomic::Ordering::Relaxed, + atomic::Ordering::Relaxed, + ) + .is_err() + { + panic!("signal iterator cannot be initialized twice"); + } + + let sa = SigAction::new( + SigHandler::Handler(handler), + SaFlags::SA_RESTART, + SigSet::empty(), + ); + + for signal in Signal::ALL { + unsafe { + nix::sys::signal::sigaction(signal.into(), &sa)?; + } + } + + Ok(Self(File::from(read))) + } +} + +impl Iterator for Signals { + type Item = io::Result; + + fn next(&mut self) -> Option { + let mut signal = [0]; + Some( + self + .0 + .read_exact(&mut signal) + .and_then(|()| Signal::try_from(signal[0])), + ) + } +} diff --git a/tests/format.rs b/tests/format.rs index 04d7d87eef..3db1aedfac 100644 --- a/tests/format.rs +++ b/tests/format.rs @@ -103,7 +103,7 @@ fn write_error() { // skip this test if running as root, since root can write files even if // permissions would otherwise forbid it #[cfg(not(windows))] - if unsafe { libc::getuid() } == 0 { + if nix::unistd::getuid() == nix::unistd::ROOT { return; } diff --git a/tests/interrupts.rs b/tests/interrupts.rs index bd0077c237..b8bbbdda6b 100644 --- a/tests/interrupts.rs +++ b/tests/interrupts.rs @@ -1,12 +1,11 @@ -use { - super::*, - std::time::{Duration, Instant}, -}; +use super::*; fn kill(process_id: u32) { - unsafe { - libc::kill(process_id.try_into().unwrap(), libc::SIGINT); - } + nix::sys::signal::kill( + nix::unistd::Pid::from_raw(process_id as i32), + nix::sys::signal::Signal::SIGINT, + ) + .unwrap(); } fn interrupt_test(arguments: &[&str], justfile: &str) { diff --git a/tests/lib.rs b/tests/lib.rs index 675e1fe048..803151af88 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -23,6 +23,7 @@ pub(crate) use { path::{Path, PathBuf, MAIN_SEPARATOR, MAIN_SEPARATOR_STR}, process::{Command, Stdio}, str, + time::{Duration, Instant}, }, tempfile::TempDir, temptree::{temptree, tree, Tree}, @@ -108,6 +109,7 @@ mod shebang; mod shell; mod shell_expansion; mod show; +mod signals; mod slash_operator; mod string; mod subsequents; diff --git a/tests/signals.rs b/tests/signals.rs new file mode 100644 index 0000000000..cc4a858782 --- /dev/null +++ b/tests/signals.rs @@ -0,0 +1,34 @@ +// todo: +// - probably first is forward sigterm to child +// - document in readme + +// - situations: +// - kill w/sigterm, sighup, sigint, sigquit +// - process cooperates / progress ignores signals +// - ctrl-c, close terminal +// - local, remote without controlling terminal, remote with controlling terminal, remote in interactive shell +// - recipe line, recipe script, --command, backtick +// +// - INT, HUP, TERM, QUIT: +// - if no children, exit immediately +// - if children, wait for them to finish, exit after they finish +// - QUIT: exit immediately? +// - TERM: forward to children +// - INFO: print info +// - get rid of or adapt interrupt tests +// +// - how to test? +// - run just +// - send signal +// - waits for child +// - child returns +// +// - tests: +// - child returns failure after signal +// - just returns signal number +// - child returns success after signal +// - just returns signal number +// - does not continue +// - TERM +// - signal forwarded to child +// - INFO prints info From bedb1a9e6fd1f067b5cdeb1e55d286783638f355 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 12:28:59 -0800 Subject: [PATCH 02/18] Remove more unsafe --- src/lib.rs | 5 ++--- src/signal_handler.rs | 3 ++- src/signals.rs | 37 ++++++++++++++++--------------------- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index dc16ed24b8..72b110d314 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,14 +57,13 @@ pub(crate) use { mem, ops::Deref, ops::{Index, Range, RangeInclusive}, - os::fd::{BorrowedFd, IntoRawFd, RawFd}, + os::fd::{BorrowedFd, IntoRawFd}, path::{self, Path, PathBuf}, process::{self, Command, ExitStatus, Stdio}, - ptr, rc::Rc, str::{self, Chars}, sync::{ - atomic::{self, AtomicBool, AtomicI32}, + atomic::{self, AtomicI32}, Mutex, MutexGuard, OnceLock, }, thread, vec, diff --git a/src/signal_handler.rs b/src/signal_handler.rs index a054e931a1..3c34504ae3 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -56,7 +56,8 @@ impl SignalHandler { Signal::Terminate => { for &child in self.children.keys() { eprintln!("sending sigterm to {child}"); - nix::sys::signal::kill(child, Some(Signal::Terminate.into())); + // todo: handle error + nix::sys::signal::kill(child, Some(Signal::Terminate.into())).ok(); } } Signal::Info => { diff --git a/src/signals.rs b/src/signals.rs index b5e5de4943..95f312fd21 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -8,37 +8,32 @@ use { static WRITE: AtomicI32 = AtomicI32::new(0); -extern "C" fn handler(signal: libc::c_int) { - let last = Errno::last(); - - let fd = WRITE.load(atomic::Ordering::Relaxed); +fn die(message: &str) -> ! { + const STDERR: BorrowedFd = unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) }; - let fd = unsafe { BorrowedFd::borrow_raw(fd) }; - - if let Err(err) = nix::unistd::write(fd, &[signal as u8]) { - // there are few times in life when one is well and truly fucked. this is - // one. - } + nix::unistd::write(STDERR, b"just: ").ok(); + nix::unistd::write(STDERR, message.as_bytes()).ok(); + nix::unistd::write(STDERR, b"\n").ok(); - last.set(); + process::abort(); } -extern "C" fn handler_old(signal: libc::c_int) { - let errno = unsafe { *libc::__error() }; +extern "C" fn handler(signal: libc::c_int) { + let errno = Errno::last(); + + let Ok(signal) = u8::try_from(signal) else { + die("unexpected signal"); + }; let buffer = &[signal as u8]; - let fd = WRITE.load(atomic::Ordering::Relaxed); + let fd = unsafe { BorrowedFd::borrow_raw(WRITE.load(atomic::Ordering::Relaxed)) }; - unsafe { - libc::write(fd, buffer.as_ptr().cast(), buffer.len()); + if nix::unistd::write(fd, buffer).is_err() { + die(Errno::last().desc()); } - // todo: should we abort if errno is bad? - - unsafe { - *libc::__error() = errno; - } + errno.set(); } pub(crate) struct Signals(File); From fae10d303b4f8aeaf818f30ba145e77b611ed6f8 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 12:39:45 -0800 Subject: [PATCH 03/18] More twiddling --- src/signals.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/signals.rs b/src/signals.rs index 95f312fd21..3cf58fec44 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -25,12 +25,11 @@ extern "C" fn handler(signal: libc::c_int) { die("unexpected signal"); }; - let buffer = &[signal as u8]; - let fd = unsafe { BorrowedFd::borrow_raw(WRITE.load(atomic::Ordering::Relaxed)) }; - if nix::unistd::write(fd, buffer).is_err() { - die(Errno::last().desc()); + if let Err(err) = nix::unistd::write(fd, &[signal]) { + let Ok(err) = Errno::try_from(err); + die(err.desc()); } errno.set(); From 903f7fae123bdaeba898db70e1c5a07ec1132037 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 12:42:03 -0800 Subject: [PATCH 04/18] Adapt --- src/signals.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/signals.rs b/src/signals.rs index 3cf58fec44..d775df8c1f 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -11,7 +11,7 @@ static WRITE: AtomicI32 = AtomicI32::new(0); fn die(message: &str) -> ! { const STDERR: BorrowedFd = unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) }; - nix::unistd::write(STDERR, b"just: ").ok(); + nix::unistd::write(STDERR, b"error: ").ok(); nix::unistd::write(STDERR, message.as_bytes()).ok(); nix::unistd::write(STDERR, b"\n").ok(); From 6c5b0e2449f2377844b8fcbf2e9ff1569961bc9f Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 14:07:36 -0800 Subject: [PATCH 05/18] Enhance --- Cargo.toml | 1 + README.md | 49 +++++++++++++++++++++++++++++++++++++++++++ src/signal_handler.rs | 13 +++++++++++- src/signals.rs | 13 +++++++++++- tests/interrupts.rs | 2 +- tests/signals.rs | 39 +++++++--------------------------- 6 files changed, 82 insertions(+), 35 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9b0bce307e..f843f5e1fe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,6 +73,7 @@ struct_excessive_bools = "allow" struct_field_names = "allow" too_many_arguments = "allow" too_many_lines = "allow" +undocumented_unsafe_blocks = "deny" unnecessary_wraps = "allow" wildcard_imports = "allow" diff --git a/README.md b/README.md index 27e3307a50..f110de4afc 100644 --- a/README.md +++ b/README.md @@ -3814,6 +3814,55 @@ the [`chrono` library docs](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) for details. +### Signal Handling + +[Signals](https://en.wikipedia.org/wiki/Signal_(IPC)) are messsages sent to +running programs to trigger specific behavior. For example, the `SIGINT` signal +is sent to all processes in the forground process group when `ctrl-c` is typed +at the terminal, + +`just` tries to exit when requested and avoid leaving behind running child +proccesses, two goals which are somewhat in conflict. + +If `just` exits leaving behind child processes, the user will have no recourse +but to `ps aux | grep` for the children and manually `kill` them, a tedious +endevour. + +#### Fatal Signals + +`SIGHUP`, `SIGINT`, and `SIGQUIT` are generated when the user closes the +terminal, types `ctrl-c`, or types `ctrl-\`, respectively, and are sent to all +processes in the foreground process group. + +`SIGTERM` is the default signal sent by the `kill` command, and is delivered +only to its intended victim. + +When a child process is not running, `just` will exit immediately on receipt of +any of the above signals. + +When a child process *is* running, `just` will wait until it terminates, to +avoid leaving it behind. + +Additionally, on receipt of `SIGTERM`, `just` will forward `SIGTERM` to any +running childrenmaster, since unlike other fatal signals, `SIGTERM`, +was likely sent to `just` alone. + +Regardless of whether a child process terminates successfully after `just` +receives a fatal signal, `just` halts execution. + +#### `SIGINFO` + +`SIGINFO` is sent to all processes in the foreground process group when the +user types `ctrl-t`. + +`just` responds by printing a list of all child process IDs and +commandsmaster. + +#### Windows + +On Windows, `just` behaves as if it had received `SIGINT` when the user types +`ctrl-c`. Other signals are unsupported. + Changelog --------- diff --git a/src/signal_handler.rs b/src/signal_handler.rs index 3c34504ae3..9f93711251 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -94,7 +94,18 @@ impl SignalHandler { Ok(child) => child, }; - let pid = Pid::from_raw(child.id() as i32); + let pid = match child.id().try_into() { + Err(err) => { + return ( + Err(io::Error::new( + io::ErrorKind::Other, + format!("invalid child PID: {err}"), + )), + None, + ) + } + Ok(pid) => Pid::from_raw(pid), + }; instance.children.insert(pid, command); diff --git a/src/signals.rs b/src/signals.rs index d775df8c1f..48971693aa 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -9,6 +9,9 @@ use { static WRITE: AtomicI32 = AtomicI32::new(0); fn die(message: &str) -> ! { + // Safety: + // + // Standard error is open for the duration of the program. const STDERR: BorrowedFd = unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) }; nix::unistd::write(STDERR, b"error: ").ok(); @@ -25,10 +28,13 @@ extern "C" fn handler(signal: libc::c_int) { die("unexpected signal"); }; + // SAFETY: + // + // `WRITE` is initialized before the signal handler can run and remains open + // for the duration of the program. let fd = unsafe { BorrowedFd::borrow_raw(WRITE.load(atomic::Ordering::Relaxed)) }; if let Err(err) = nix::unistd::write(fd, &[signal]) { - let Ok(err) = Errno::try_from(err); die(err.desc()); } @@ -60,6 +66,11 @@ impl Signals { ); for signal in Signal::ALL { + // SAFETY: + // + // This is the only place we modify signal handlers, and + // `nix::sys::signal::sigaction` is unsafe only if an invalid signal + // handler has already been installed. unsafe { nix::sys::signal::sigaction(signal.into(), &sa)?; } diff --git a/tests/interrupts.rs b/tests/interrupts.rs index b8bbbdda6b..f0b9ff59a7 100644 --- a/tests/interrupts.rs +++ b/tests/interrupts.rs @@ -2,7 +2,7 @@ use super::*; fn kill(process_id: u32) { nix::sys::signal::kill( - nix::unistd::Pid::from_raw(process_id as i32), + nix::unistd::Pid::from_raw(process_id.try_into().unwrap()), nix::sys::signal::Signal::SIGINT, ) .unwrap(); diff --git a/tests/signals.rs b/tests/signals.rs index cc4a858782..30652c5dae 100644 --- a/tests/signals.rs +++ b/tests/signals.rs @@ -1,34 +1,9 @@ // todo: -// - probably first is forward sigterm to child -// - document in readme - -// - situations: -// - kill w/sigterm, sighup, sigint, sigquit -// - process cooperates / progress ignores signals -// - ctrl-c, close terminal -// - local, remote without controlling terminal, remote with controlling terminal, remote in interactive shell -// - recipe line, recipe script, --command, backtick -// -// - INT, HUP, TERM, QUIT: -// - if no children, exit immediately -// - if children, wait for them to finish, exit after they finish -// - QUIT: exit immediately? -// - TERM: forward to children -// - INFO: print info -// - get rid of or adapt interrupt tests -// -// - how to test? -// - run just -// - send signal -// - waits for child -// - child returns -// +// - create issue for sighup handling: can we detect when we receive sighup and are the only recipient? // - tests: -// - child returns failure after signal -// - just returns signal number -// - child returns success after signal -// - just returns signal number -// - does not continue -// - TERM -// - signal forwarded to child -// - INFO prints info +// - just terminates immediately on receipt of fatal signal if no child process is running +// - just reports correct error if child process is terminated with fatal signal and indicates failure +// - just still terminates if child process is terminated with fatal signal and reports success +// - just prints info receipt of SIGINFO +// - contexts: recipe line, recipe script, --command, backtick (any others?) +// - get rid of old interrupt tests From 04f78bc9744c55241099ce6f7da85f49b397b638 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 14:17:15 -0800 Subject: [PATCH 06/18] Add horrific cfg blocks --- README.md | 4 +++- src/signal.rs | 40 ++++++++++++++++++++++++++++++++++++++++ src/signal_handler.rs | 16 ++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f110de4afc..12a8704c23 100644 --- a/README.md +++ b/README.md @@ -3853,7 +3853,9 @@ receives a fatal signal, `just` halts execution. #### `SIGINFO` `SIGINFO` is sent to all processes in the foreground process group when the -user types `ctrl-t`. +user types `ctrl-t` on +[BSD](https://en.wikipedia.org/wiki/Berkeley_Software_Distribution)-derived +operating systems, including MacOS, but not Linux. `just` responds by printing a list of all child process IDs and commandsmaster. diff --git a/src/signal.rs b/src/signal.rs index 9ffafcefd4..9b572fbc6a 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -4,6 +4,14 @@ use super::*; #[repr(i32)] pub(crate) enum Signal { Hangup = libc::SIGHUP, + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] Info = libc::SIGINFO, Interrupt = libc::SIGINT, Quit = libc::SIGQUIT, @@ -13,6 +21,14 @@ pub(crate) enum Signal { impl Signal { pub(crate) const ALL: [Signal; 5] = [ Signal::Hangup, + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] Signal::Info, Signal::Interrupt, Signal::Quit, @@ -31,6 +47,14 @@ impl Display for Signal { "{}", match self { Signal::Hangup => "SIGHUP", + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] Signal::Info => "SIGINFO", Signal::Interrupt => "SIGINT", Signal::Quit => "SIGQUIT", @@ -44,6 +68,14 @@ impl From for nix::sys::signal::Signal { fn from(signal: Signal) -> Self { match signal { Signal::Hangup => Self::SIGHUP, + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] Signal::Info => Self::SIGINFO, Signal::Interrupt => Self::SIGINT, Signal::Quit => Self::SIGQUIT, @@ -58,6 +90,14 @@ impl TryFrom for Signal { fn try_from(n: u8) -> Result { match n.into() { libc::SIGHUP => Ok(Signal::Hangup), + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] libc::SIGINFO => Ok(Signal::Info), libc::SIGINT => Ok(Signal::Interrupt), libc::SIGQUIT => Ok(Signal::Quit), diff --git a/src/signal_handler.rs b/src/signal_handler.rs index 9f93711251..ba79375ba4 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -46,6 +46,14 @@ impl SignalHandler { process::exit(128 + signal.number()); } + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] if signal != Signal::Info && self.caught.is_none() { self.caught = Some(signal); } @@ -60,6 +68,14 @@ impl SignalHandler { nix::sys::signal::kill(child, Some(Signal::Terminate.into())).ok(); } } + #[cfg(any( + target_os = "dragonfly", + target_os = "freebsd", + target_os = "ios", + target_os = "macos", + target_os = "netbsd", + target_os = "openbsd", + ))] Signal::Info => { // todo: print pid if self.children.is_empty() { From b2d83efb55e51867a476db7c582385dc2f01125d Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 14:18:58 -0800 Subject: [PATCH 07/18] Adapt --- src/signal.rs | 2 +- src/signals.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/signal.rs b/src/signal.rs index 9b572fbc6a..7e916b0653 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -19,7 +19,7 @@ pub(crate) enum Signal { } impl Signal { - pub(crate) const ALL: [Signal; 5] = [ + pub(crate) const ALL: &[Signal] = &[ Signal::Hangup, #[cfg(any( target_os = "dragonfly", diff --git a/src/signals.rs b/src/signals.rs index 48971693aa..dc3ac4e362 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -65,7 +65,7 @@ impl Signals { SigSet::empty(), ); - for signal in Signal::ALL { + for &signal in Signal::ALL { // SAFETY: // // This is the only place we modify signal handlers, and From 6c15e8b91e6fdac36e4d9e3f7a581f3214b70a99 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 14:26:06 -0800 Subject: [PATCH 08/18] Reform --- tests/signals.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/signals.rs b/tests/signals.rs index 30652c5dae..8d69861d5b 100644 --- a/tests/signals.rs +++ b/tests/signals.rs @@ -1,5 +1,6 @@ // todo: // - create issue for sighup handling: can we detect when we receive sighup and are the only recipient? +// - fix on windows // - tests: // - just terminates immediately on receipt of fatal signal if no child process is running // - just reports correct error if child process is terminated with fatal signal and indicates failure From 04a58aec4cd9313f1c875ee5325427b3b87016ca Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 15:07:31 -0800 Subject: [PATCH 09/18] Fix TODOs --- src/signal_handler.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/signal_handler.rs b/src/signal_handler.rs index ba79375ba4..ea4fff561b 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -63,8 +63,9 @@ impl SignalHandler { // processes and wait for them to exit Signal::Terminate => { for &child in self.children.keys() { - eprintln!("sending sigterm to {child}"); - // todo: handle error + if self.verbosity.loquacious() { + eprintln!("just: sending SIGTERM to child process {child}"); + } nix::sys::signal::kill(child, Some(Signal::Terminate.into())).ok(); } } @@ -77,12 +78,15 @@ impl SignalHandler { target_os = "openbsd", ))] Signal::Info => { - // todo: print pid if self.children.is_empty() { eprintln!("just: no child processes"); } else { - // todo: handle plural correctly - let mut message = format!("just: {} child processes:\n", self.children.len()); + let n = self.children.len(); + + let mut message = format!( + "just: {n} child {}:\n", + if n == 1 { "process" } else { "processes" } + ); for (&child, command) in &self.children { message.push_str(&format!("{child}: {command:?}\n")); From fa29e2274a748f9d99bf8d4dca71fe2b114cdfb4 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 15:09:48 -0800 Subject: [PATCH 10/18] print PID on SIGINFO --- src/signal_handler.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/signal_handler.rs b/src/signal_handler.rs index ea4fff561b..c911695ec2 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -59,16 +59,11 @@ impl SignalHandler { } match signal { - // SIGTERM is the default signal sent by kill. forward it to child - // processes and wait for them to exit - Signal::Terminate => { - for &child in self.children.keys() { - if self.verbosity.loquacious() { - eprintln!("just: sending SIGTERM to child process {child}"); - } - nix::sys::signal::kill(child, Some(Signal::Terminate.into())).ok(); - } - } + // SIGHUP, SIGINT, and SIGQUIT are normally sent on terminal close, + // ctrl-c, and ctrl-\, respectively, and are sent to all processes in the + // foreground process group. this includes child processes, so we ignore + // the signal and rely on them to exit + Signal::Hangup | Signal::Interrupt | Signal::Quit => {} #[cfg(any( target_os = "dragonfly", target_os = "freebsd", @@ -78,13 +73,14 @@ impl SignalHandler { target_os = "openbsd", ))] Signal::Info => { + let id = process::id(); if self.children.is_empty() { - eprintln!("just: no child processes"); + eprintln!("just {id}: no child processes"); } else { let n = self.children.len(); let mut message = format!( - "just: {n} child {}:\n", + "just {id}: {n} child {}:\n", if n == 1 { "process" } else { "processes" } ); @@ -95,11 +91,16 @@ impl SignalHandler { eprint!("{message}"); } } - // SIGHUP, SIGINT, and SIGQUIT are normally sent on terminal close, - // ctrl-c, and ctrl-\, respectively, and are sent to all processes in the - // foreground process group. this includes child processes, so we ignore - // the signal and rely on them to exit - Signal::Hangup | Signal::Interrupt | Signal::Quit => {} + // SIGTERM is the default signal sent by kill. forward it to child + // processes and wait for them to exit + Signal::Terminate => { + for &child in self.children.keys() { + if self.verbosity.loquacious() { + eprintln!("just: sending SIGTERM to child process {child}"); + } + nix::sys::signal::kill(child, Some(Signal::Terminate.into())).ok(); + } + } } } From ea3ce66919374019101409ba64300fdaa3eabc74 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 15:27:22 -0800 Subject: [PATCH 11/18] Adjust --- src/platform.rs | 1 + tests/signals.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform.rs b/src/platform.rs index b393a1dfd5..de1db44062 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -48,6 +48,7 @@ impl PlatformInterface for Platform { fn install_signal_handler(handler: T) -> RunResult<'static> { thread::spawn(move || { for signal in Signals::new().unwrap() { + // todo: handle error? handler(signal.unwrap()); } }); diff --git a/tests/signals.rs b/tests/signals.rs index 8d69861d5b..8b12636dba 100644 --- a/tests/signals.rs +++ b/tests/signals.rs @@ -1,6 +1,6 @@ // todo: // - create issue for sighup handling: can we detect when we receive sighup and are the only recipient? -// - fix on windows +// - fix and test on windows // - tests: // - just terminates immediately on receipt of fatal signal if no child process is running // - just reports correct error if child process is terminated with fatal signal and indicates failure From 88a5ff9177434d21debfad3511fff12c9a369547 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 15:27:29 -0800 Subject: [PATCH 12/18] Modify --- src/platform.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform.rs b/src/platform.rs index de1db44062..7548625976 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -47,8 +47,8 @@ impl PlatformInterface for Platform { fn install_signal_handler(handler: T) -> RunResult<'static> { thread::spawn(move || { + // todo: handle errors? for signal in Signals::new().unwrap() { - // todo: handle error? handler(signal.unwrap()); } }); From 26fa29b3df030aec00d072e2c99b624834e190ba Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Fri, 29 Nov 2024 15:27:47 -0800 Subject: [PATCH 13/18] Tweak --- src/platform.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/platform.rs b/src/platform.rs index 7548625976..d7a677d0a7 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -46,6 +46,7 @@ impl PlatformInterface for Platform { } fn install_signal_handler(handler: T) -> RunResult<'static> { + // todo: name thread thread::spawn(move || { // todo: handle errors? for signal in Signals::new().unwrap() { From b51d56774ea2fb053eaf7526f1d2c46918765c77 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Sat, 30 Nov 2024 00:24:30 -0800 Subject: [PATCH 14/18] Use -1 as invalid file number --- src/signals.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/signals.rs b/src/signals.rs index dc3ac4e362..9839463e5e 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -6,7 +6,9 @@ use { }, }; -static WRITE: AtomicI32 = AtomicI32::new(0); +const INVALID_FILENO: i32 = -1; + +static WRITE: AtomicI32 = AtomicI32::new(INVALID_FILENO); fn die(message: &str) -> ! { // Safety: @@ -49,7 +51,7 @@ impl Signals { if WRITE .compare_exchange( - 0, + INVALID_FILENO, write.into_raw_fd(), atomic::Ordering::Relaxed, atomic::Ordering::Relaxed, From 3d6952c3abe7e9d3d6372745dbdd3168171750c2 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Sat, 30 Nov 2024 00:28:50 -0800 Subject: [PATCH 15/18] Use consistant case for SAFETY --- src/signals.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/signals.rs b/src/signals.rs index 9839463e5e..240aed497b 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -11,7 +11,7 @@ const INVALID_FILENO: i32 = -1; static WRITE: AtomicI32 = AtomicI32::new(INVALID_FILENO); fn die(message: &str) -> ! { - // Safety: + // SAFETY: // // Standard error is open for the duration of the program. const STDERR: BorrowedFd = unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) }; From d17ec603dee65a5144ab9da9d4b7398752f8851c Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Sat, 30 Nov 2024 00:33:31 -0800 Subject: [PATCH 16/18] Tweak --- src/signal_handler.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/signal_handler.rs b/src/signal_handler.rs index c911695ec2..688bb74ad6 100644 --- a/src/signal_handler.rs +++ b/src/signal_handler.rs @@ -62,7 +62,7 @@ impl SignalHandler { // SIGHUP, SIGINT, and SIGQUIT are normally sent on terminal close, // ctrl-c, and ctrl-\, respectively, and are sent to all processes in the // foreground process group. this includes child processes, so we ignore - // the signal and rely on them to exit + // the signal and wait for them to exit Signal::Hangup | Signal::Interrupt | Signal::Quit => {} #[cfg(any( target_os = "dragonfly", From fbf3320f401abb0abd170a5b6b89d61057977821 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Sat, 21 Dec 2024 14:07:38 -0800 Subject: [PATCH 17/18] Write error message with a single call to write to avoid interleaving --- src/signals.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/signals.rs b/src/signals.rs index 240aed497b..e6302a6c2e 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -16,9 +16,22 @@ fn die(message: &str) -> ! { // Standard error is open for the duration of the program. const STDERR: BorrowedFd = unsafe { BorrowedFd::borrow_raw(libc::STDERR_FILENO) }; - nix::unistd::write(STDERR, b"error: ").ok(); - nix::unistd::write(STDERR, message.as_bytes()).ok(); - nix::unistd::write(STDERR, b"\n").ok(); + let mut i = 0; + let mut buffer = [0; 512]; + + let mut append = |s: &str| { + let remaining = buffer.len() - i; + let n = s.len().min(remaining); + let end = i + n; + buffer[i..end].copy_from_slice(&s.as_bytes()[0..n]); + i = end; + }; + + append("error: "); + append(message); + append("\n"); + + nix::unistd::write(STDERR, &buffer[0..i]).ok(); process::abort(); } From 5f172373d5c02712daac7db570e596a8d8f4ddf9 Mon Sep 17 00:00:00 2001 From: Casey Rodarmor Date: Mon, 23 Dec 2024 10:17:48 -0800 Subject: [PATCH 18/18] Don't exit if infallible line catches signal --- src/recipe.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/recipe.rs b/src/recipe.rs index bccf485037..e275ec3dac 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -310,11 +310,13 @@ impl<'src, D> Recipe<'src, D> { }); } } else { - return Err(error_from_signal( - self.name(), - Some(line_number), - exit_status, - )); + if !infallible_line { + return Err(error_from_signal( + self.name(), + Some(line_number), + exit_status, + )); + } } } Err(io_error) => { @@ -325,8 +327,10 @@ impl<'src, D> Recipe<'src, D> { } }; - if let Some(signal) = caught { - return Err(Error::Interrupted { signal }); + if !infallible_line { + if let Some(signal) = caught { + return Err(Error::Interrupted { signal }); + } } } }