Skip to content

Commit

Permalink
Improve signal/interrupt handling
Browse files Browse the repository at this point in the history
- Renamed the interrupt_handler to signal_handler to make it more explicit that
any arbitrary signal can be handled
- Replace ctrlc with signal-hook to be able to handle specific signals, and be
able to propagate them transparently
  - A substantial change was done to handle continuously polling the process via
  child.try_wait, instead of handler that was used previously with ctrlc
- Fix error codes when recipes/commands/shebangs/inline are terminated via a
signal
- Add command-group to be able to handle shebang command executions which may
cause sub-processes to be spawned
- Re-enable signal/interrupt tests and add a test that validates that multiple
signals can be sent if the program is able to trap them
  • Loading branch information
davoclavo committed Jan 12, 2024
1 parent 53cea2f commit 91162d5
Show file tree
Hide file tree
Showing 17 changed files with 366 additions and 264 deletions.
52 changes: 41 additions & 11 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"] }
Expand Down
2 changes: 1 addition & 1 deletion src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
}

Expand Down
8 changes: 3 additions & 5 deletions src/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}

Expand Down
16 changes: 0 additions & 16 deletions src/interrupt_guard.rs

This file was deleted.

86 changes: 0 additions & 86 deletions src/interrupt_handler.rs

This file was deleted.

10 changes: 4 additions & 6 deletions src/justfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
20 changes: 9 additions & 11 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -152,7 +150,6 @@ mod loader;
mod name;
mod namepath;
mod ordinal;
mod output;
mod output_error;
mod parameter;
mod parameter_kind;
Expand All @@ -177,6 +174,7 @@ mod settings;
mod shebang;
mod shell;
mod show_whitespace;
mod signal_handler;
mod source;
mod string_kind;
mod string_literal;
Expand Down
34 changes: 0 additions & 34 deletions src/output.rs

This file was deleted.

4 changes: 4 additions & 0 deletions src/platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> {
path
.to_str()
Expand Down
5 changes: 5 additions & 0 deletions src/platform_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ pub(crate) trait PlatformInterface {
/// signal
fn signal_from_exit_status(exit_status: ExitStatus) -> Option<i32>;

/// 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<String, String>;
}
4 changes: 2 additions & 2 deletions src/recipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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| {
Expand Down
Loading

0 comments on commit 91162d5

Please sign in to comment.