diff --git a/Cargo.lock b/Cargo.lock index 17bc25c63e..f015d85223 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -69,6 +69,15 @@ version = "1.1.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c59e92b5a388f549b863a7bea62612c09f24c8393560709a54558a9abdfb3b9c" +[[package]] +name = "cc" +version = "1.0.83" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f1174fb0b6ec23863f8b971027804a42614e347eafb0a95bf0b12cdae21fc4d0" +dependencies = [ + "libc", +] + [[package]] name = "cfg-if" version = "1.0.0" @@ -91,6 +100,16 @@ dependencies = [ "vec_map", ] +[[package]] +name = "command-group" +version = "5.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a68fa787550392a9d58f44c21a3022cfb3ea3e2458b7f85d3b399d0ceeccf409" +dependencies = [ + "nix", + "winapi", +] + [[package]] name = "cpufeatures" version = "0.2.12" @@ -119,16 +138,6 @@ dependencies = [ "typenum", ] -[[package]] -name = "ctrlc" -version = "3.4.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b467862cc8610ca6fc9a1532d7777cee0804e678ab45410897b9396495994a0b" -dependencies = [ - "nix", - "windows-sys 0.52.0", -] - [[package]] name = "derivative" version = "2.2.0" @@ -338,8 +347,8 @@ dependencies = [ "atty", "camino", "clap", + "command-group", "cradle", - "ctrlc", "derivative", "dirs", "dotenvy", @@ -357,6 +366,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "signal-hook", "similar", "snafu", "strum", @@ -666,6 +676,26 @@ dependencies = [ "digest", ] +[[package]] +name = "signal-hook" +version = "0.3.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8621587d4798caf8eb44879d42e56b9a93ea5dcd315a6487c357130095b62801" +dependencies = [ + "cc", + "libc", + "signal-hook-registry", +] + +[[package]] +name = "signal-hook-registry" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d8229b473baa5980ac72ef434c4415e70c4b5e71b423043adb4ba059f89c99a1" +dependencies = [ + "libc", +] + [[package]] name = "similar" version = "2.4.0" diff --git a/Cargo.toml b/Cargo.toml index b353b5a22b..9b72f24c5e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ ansi_term = "0.12.0" atty = "0.2.0" camino = "1.0.4" clap = { version = "2.33.0", features = ["wrap_help"] } -ctrlc = { version = "3.1.1", features = ["termination"] } +command-group = "5.0.1" derivative = "2.0.0" dirs = "5.0.1" dotenvy = "0.15" @@ -38,6 +38,7 @@ semver = "1.0.20" serde = { version = "1.0.130", features = ["derive", "rc"] } serde_json = "1.0.68" sha2 = "0.10" +signal-hook = {version = "0.3.17", features = ["extended-siginfo"]} similar = { version = "2.1.0", features = ["unicode"] } snafu = "0.8.0" strum = { version = "0.25.0", features = ["derive"] } diff --git a/src/config.rs b/src/config.rs index 0a0714635e..2a92823ea0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -682,7 +682,7 @@ impl Config { } pub(crate) fn run(self, loader: &Loader) -> Result<(), Error> { - if let Err(error) = InterruptHandler::install(self.verbosity) { + if let Err(error) = SignalHandler::install(self.verbosity) { warn!("Failed to set CTRL-C handler: {error}"); } diff --git a/src/evaluator.rs b/src/evaluator.rs index c768625d1e..5ea3085dcb 100644 --- a/src/evaluator.rs +++ b/src/evaluator.rs @@ -218,11 +218,9 @@ impl<'src, 'run> Evaluator<'src, 'run> { Stdio::inherit() }); - InterruptHandler::guard(|| { - output(cmd).map_err(|output_error| Error::Backtick { - token: *token, - output_error, - }) + SignalHandler::guard_output(cmd).map_err(|output_error| Error::Backtick { + token: *token, + output_error, }) } 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 84e1c2be61..2563bf15d2 100644 --- a/src/justfile.rs +++ b/src/justfile.rs @@ -159,12 +159,10 @@ impl<'src> Justfile<'src> { command.export(&self.settings, &dotenv, &scope); - let status = InterruptHandler::guard(|| command.status()).map_err(|io_error| { - Error::CommandInvoke { - binary: binary.clone(), - arguments: arguments.clone(), - io_error, - } + let status = SignalHandler::guard(command).map_err(|io_error| Error::CommandInvoke { + binary: binary.clone(), + arguments: arguments.clone(), + io_error, })?; if !status.success() { diff --git a/src/lib.rs b/src/lib.rs index 445e819019..2835ccd682 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -22,20 +22,20 @@ pub(crate) use { conditional_operator::ConditionalOperator, config::Config, config_error::ConfigError, count::Count, delimiter::Delimiter, dependency::Dependency, dump_format::DumpFormat, enclosure::Enclosure, error::Error, evaluator::Evaluator, expression::Expression, - fragment::Fragment, function::Function, function_context::FunctionContext, - interrupt_guard::InterruptGuard, interrupt_handler::InterruptHandler, item::Item, + fragment::Fragment, function::Function, function_context::FunctionContext, item::Item, justfile::Justfile, keyed::Keyed, keyword::Keyword, lexer::Lexer, line::Line, list::List, load_dotenv::load_dotenv, loader::Loader, name::Name, namepath::Namepath, ordinal::Ordinal, - output::output, output_error::OutputError, parameter::Parameter, parameter_kind::ParameterKind, - parser::Parser, platform::Platform, platform_interface::PlatformInterface, position::Position, + 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_context::RecipeContext, recipe_resolver::RecipeResolver, scope::Scope, search::Search, search_config::SearchConfig, search_error::SearchError, set::Set, setting::Setting, settings::Settings, shebang::Shebang, shell::Shell, show_whitespace::ShowWhitespace, - source::Source, 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, - use_color::UseColor, variables::Variables, verbosity::Verbosity, warning::Warning, + signal_handler::SignalHandler, source::Source, 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, use_color::UseColor, variables::Variables, + verbosity::Verbosity, warning::Warning, }, std::{ cmp, @@ -138,8 +138,6 @@ mod expression; mod fragment; mod function; mod function_context; -mod interrupt_guard; -mod interrupt_handler; mod item; mod justfile; mod keyed; @@ -152,7 +150,6 @@ mod loader; mod name; mod namepath; mod ordinal; -mod output; mod output_error; mod parameter; mod parameter_kind; @@ -177,6 +174,7 @@ mod settings; mod shebang; mod shell; mod show_whitespace; +mod signal_handler; mod source; mod string_kind; mod string_literal; diff --git a/src/output.rs b/src/output.rs deleted file mode 100644 index af8ec6a862..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(utf8) => Ok( - if utf8.ends_with('\n') { - &utf8[0..utf8.len() - 1] - } else if utf8.ends_with("\r\n") { - &utf8[0..utf8.len() - 2] - } else { - utf8 - } - .to_owned(), - ), - } - } - Err(io_error) => Err(OutputError::Io(io_error)), - } -} diff --git a/src/platform.rs b/src/platform.rs index 36be7fc743..e9a431dbfd 100644 --- a/src/platform.rs +++ b/src/platform.rs @@ -38,6 +38,10 @@ impl PlatformInterface for Platform { exit_status.signal() } + fn exit_code_from_signal(signal: i32) -> i32 { + signal + 128 + } + fn convert_native_path(_working_directory: &Path, path: &Path) -> Result { path .to_str() diff --git a/src/platform_interface.rs b/src/platform_interface.rs index 4a984ffb5d..fa59cd1410 100644 --- a/src/platform_interface.rs +++ b/src/platform_interface.rs @@ -16,6 +16,11 @@ pub(crate) trait PlatformInterface { /// signal fn signal_from_exit_status(exit_status: ExitStatus) -> Option; + /// Convert a signal into an exit code, for the case where a process was + /// terminated by a signal + /// See https://tldp.org/LDP/abs/html/exitcodes.html + fn exit_code_from_signal(signal: i32) -> i32; + /// Translate a path from a "native" path to a path the interpreter expects fn convert_native_path(working_directory: &Path, path: &Path) -> Result; } diff --git a/src/recipe.rs b/src/recipe.rs index 99f90e1f61..fc4316e928 100644 --- a/src/recipe.rs +++ b/src/recipe.rs @@ -254,7 +254,7 @@ impl<'src, D> Recipe<'src, D> { cmd.export(context.settings, dotenv, scope); - match InterruptHandler::guard(|| cmd.status()) { + match SignalHandler::guard(cmd) { Ok(exit_status) => { if let Some(code) = exit_status.code() { if code != 0 && !infallible_command { @@ -384,7 +384,7 @@ impl<'src, D> Recipe<'src, D> { command.export(context.settings, dotenv, scope); // run it! - match InterruptHandler::guard(|| command.status()) { + match SignalHandler::guard(command) { Ok(exit_status) => exit_status.code().map_or_else( || Err(error_from_signal(self.name(), None, exit_status)), |code| { diff --git a/src/run.rs b/src/run.rs index 764440ea1c..e5b5e12699 100644 --- a/src/run.rs +++ b/src/run.rs @@ -33,6 +33,22 @@ pub fn run() -> Result<(), i32> { if !verbosity.quiet() && error.print_message() { eprintln!("{}", error.color_display(color.stderr())); } - error.code().unwrap_or(EXIT_FAILURE) + match error.code() { + Some(code) => code, + None => match error { + Error::Signal { signal, .. } + | Error::Backtick { + output_error: OutputError::Signal(signal), + .. + } => Platform::exit_code_from_signal(signal), + Error::CommandStatus { status, .. } => match status.code() { + Some(code) => code, + None => Platform::signal_from_exit_status(status) + .map(Platform::exit_code_from_signal) + .unwrap_or(EXIT_FAILURE), + }, + _ => EXIT_FAILURE, + }, + } }) } diff --git a/src/signal_handler.rs b/src/signal_handler.rs new file mode 100644 index 0000000000..9741f7d8c4 --- /dev/null +++ b/src/signal_handler.rs @@ -0,0 +1,154 @@ +use super::*; + +use command_group::{CommandGroup, GroupChild}; +use signal_hook::consts::signal::*; +use signal_hook::consts::TERM_SIGNALS; +use signal_hook::flag; +use signal_hook::iterator::exfiltrator::WithOrigin; +use signal_hook::iterator::SignalsInfo; +use std::io::Read; +use std::process::Command; +use std::sync::{atomic::AtomicBool, Arc}; + +pub(crate) struct SignalHandler { + verbosity: Verbosity, + signals: Option>, +} + +impl SignalHandler { + pub(crate) fn install(verbosity: Verbosity) -> Result<(), std::io::Error> { + let mut instance = Self::instance(); + instance.verbosity = verbosity; + + let signal_flag = Arc::new(AtomicBool::new(false)); + + const MISC_SIGNALS: &[i32] = &[ + #[cfg(not(windows))] + SIGHUP, + // SIGABRT, + // SIGPIPE, + // SIGALRM, + ]; + + let mut sigs = MISC_SIGNALS.to_vec(); + sigs.extend(TERM_SIGNALS); + + let signals = SignalsInfo::::new(&sigs)?; + instance.signals = Some(signals); + + for sig in TERM_SIGNALS { + // NOTE This could be set before the register handler, in order to terminate in a double ctrl-c + // flag::register_conditional_shutdown(*sig, 1, Arc::clone(&signal_flag))?; + flag::register(*sig, Arc::clone(&signal_flag))?; + } + + for sig in MISC_SIGNALS { + flag::register(*sig, Arc::clone(&signal_flag))?; + } + Ok(()) + } + + 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!("interrupt handler mutex poisoned: {poison_error}"), + } + .color_display(Color::auto().stderr()) + ); + process::exit(EXIT_FAILURE); + } + } + } + + const fn new() -> Self { + Self { + verbosity: Verbosity::default(), + signals: None, + } + } + + fn wait_child(mut child: GroupChild) -> std::io::Result<(GroupChild, ExitStatus)> { + let mut instance = Self::instance(); + loop { + if let Some(signals) = instance.signals.as_mut() { + for signal_info in signals.pending() { + unsafe { + // Send signal to the child process group, thus the negative pid + libc::kill(-(child.id() as i32), signal_info.signal); + } + } + } + match child.try_wait() { + Ok(None) => { + std::thread::sleep(std::time::Duration::from_millis(10)); + // If the child process is still running, continue polling + continue; + } + Ok(Some(status)) => { + // If the child process has exited, break the loop + return Ok((child, status)); + } + Err(e) => { + return Err(e); + } + } + } + } + + pub(crate) fn guard_output(mut command: Command) -> Result { + let child = command + .stdout(std::process::Stdio::piped()) + .group_spawn() + .unwrap(); + match Self::wait_child(child) { + Ok((mut child, status)) => { + if let Some(code) = status.code() { + if code != 0 { + return Err(OutputError::Code(code)); + } + } else { + let signal = Platform::signal_from_exit_status(status); + return Err(match signal { + Some(signal) => OutputError::Signal(signal), + None => OutputError::Unknown, + }); + } + match child.inner().stdout.as_mut() { + Some(stdout) => { + let mut buffer = Vec::new(); + stdout.read_to_end(&mut buffer).unwrap(); + match str::from_utf8(buffer.as_slice()) { + Err(error) => Err(OutputError::Utf8(error)), + Ok(utf8) => Ok( + if utf8.ends_with('\n') { + &utf8[0..utf8.len() - 1] + } else if utf8.ends_with("\r\n") { + &utf8[0..utf8.len() - 2] + } else { + utf8 + } + .to_owned(), + ), + } + } + None => Err(OutputError::Unknown), + } + } + Err(error) => Err(OutputError::Io(error)), + } + } + + pub(crate) fn guard(mut command: Command) -> Result { + let child = command.group_spawn().unwrap(); + match Self::wait_child(child) { + Ok((_, status)) => Ok(status), + Err(error) => Err(error), + } + } +} diff --git a/tests/interrupts.rs b/tests/interrupts.rs deleted file mode 100644 index 6df78c212d..0000000000 --- a/tests/interrupts.rs +++ /dev/null @@ -1,88 +0,0 @@ -use { - super::*, - std::time::{Duration, Instant}, -}; - -fn kill(process_id: u32) { - unsafe { - libc::kill(process_id as i32, libc::SIGINT); - } -} - -fn interrupt_test(arguments: &[&str], justfile: &str) { - let tmp = tempdir(); - let mut justfile_path = tmp.path().to_path_buf(); - justfile_path.push("justfile"); - fs::write(justfile_path, unindent(justfile)).unwrap(); - - let start = Instant::now(); - - let mut child = Command::new(executable_path("just")) - .current_dir(&tmp) - .args(arguments) - .spawn() - .expect("just invocation failed"); - - while start.elapsed() < Duration::from_millis(500) {} - - kill(child.id()); - - let status = child.wait().unwrap(); - - let elapsed = start.elapsed(); - - if elapsed > Duration::from_secs(2) { - panic!("process returned too late: {elapsed:?}"); - } - - if elapsed < Duration::from_millis(100) { - panic!("process returned too early : {elapsed:?}"); - } - - assert_eq!(status.code(), Some(130)); -} - -#[test] -#[ignore] -fn interrupt_shebang() { - interrupt_test( - &[], - " - default: - #!/usr/bin/env sh - sleep 1 - ", - ); -} - -#[test] -#[ignore] -fn interrupt_line() { - interrupt_test( - &[], - " - default: - @sleep 1 - ", - ); -} - -#[test] -#[ignore] -fn interrupt_backtick() { - interrupt_test( - &[], - " - foo := `sleep 1` - - default: - @echo {{foo}} - ", - ); -} - -#[test] -#[ignore] -fn interrupt_command() { - interrupt_test(&["--command", "sleep", "1"], ""); -} diff --git a/tests/lib.rs b/tests/lib.rs index a9f3c3448f..4f421decae 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -58,8 +58,6 @@ mod functions; mod ignore_comments; mod imports; mod init; -#[cfg(unix)] -mod interrupts; mod invocation_directory; mod json; mod line_prefixes; @@ -86,6 +84,8 @@ mod shadowing_parameters; mod shebang; mod shell; mod show; +#[cfg(unix)] +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..117b412096 --- /dev/null +++ b/tests/signals.rs @@ -0,0 +1,122 @@ +use { + super::*, + libc::{SIGHUP, SIGINT}, + std::time::{Duration, Instant}, +}; + +fn send_signal(process_id: u32, signal: i32) { + unsafe { + libc::kill(process_id as i32, signal); + } +} + +fn signal_test(arguments: &[&str], justfile: &str, times: u64) { + let tmp = tempdir(); + let mut justfile_path = tmp.path().to_path_buf(); + justfile_path.push("justfile"); + fs::write(justfile_path, unindent(justfile)).unwrap(); + + let signals = [SIGINT, SIGHUP]; + for signal in signals { + let start = Instant::now(); + let mut child = Command::new(executable_path("just")) + .current_dir(&tmp) + .args(arguments) + .spawn() + .expect("just invocation failed"); + + let initial_wait = 500; + let cycle_wait = 50; + while start.elapsed() < Duration::from_millis(initial_wait) {} + + for i in 0..times { + // wait a little bit each time we send a signal + while start.elapsed() < Duration::from_millis(initial_wait + cycle_wait * (i + 1)) {} + send_signal(child.id(), signal); + } + + let status = child.wait().expect("failed to wait on child"); + + let elapsed = start.elapsed(); + + if elapsed > Duration::from_millis(1000) { + panic!("process returned too late: {elapsed:?}"); + } + + if elapsed < Duration::from_millis(initial_wait + cycle_wait * times) { + panic!("process returned too early : {elapsed:?}"); + } + + assert_eq!(status.code(), Some(signal + 128)); + } +} + +#[test] +fn signal_shebang() { + signal_test( + &[], + " + default: + #!/usr/bin/env sh + + sleep 1 + ", + 1, + ); +} + +#[test] +fn signal_line() { + signal_test( + &[], + " + default: + @sleep 1 + ", + 1, + ); +} + +#[test] +fn signal_backtick() { + signal_test( + &[], + " + foo := `sleep 1` + + default: + @echo {{foo}} + ", + 1, + ); +} + +#[test] +fn signal_command() { + signal_test(&["--command", "sleep", "1"], "", 1); +} + +#[test] +fn multiple_signals_shebang() { + signal_test( + &[], + " + default: + #!/usr/bin/env sh + + counter=0 + handle_signal() { + ((counter++)) + if ((counter > 5)); then + exit $1 + fi + } + + trap 'handle_signal 129' SIGHUP + trap 'handle_signal 130' SIGINT + + sleep 1 + ", + 5, + ); +}