Skip to content

Commit

Permalink
helix-term: send the STOP signal to all processes in the process group (
Browse files Browse the repository at this point in the history
helix-editor#3546)

* helix-term: send the STOP signal to all processes in the process group

From kill(3p):

    If pid is 0, sig shall be sent to all processes (excluding an unspecified set
    of  system processes) whose process group ID is equal to the process group ID
    of the sender, and for which the process has permission to send a signal.

This fixes the issue of running `git commit`, attempting to suspend
helix with ^Z, and then not regaining control over the terminal and
having to press ^Z again.

* helix-term: use libc directly to send STOP signal

* helix-term: document safety of libc::kill

* helix-term: properly handle libc::kill's failure

I misread the manpage for POSIX `kill` -- it returns `-1` in
the failure case, and sets `errno`, which is retrieved via
`std::io::Error::last_os_error()`, has its string representation printed
out, and then exits with the matching status code (or 1 if, for whatever
reason, there is no matching status code).

* helix-term: expand upon why we need to SIGSTOP the entire process group

Also add a link back to one of the upstream issues.
  • Loading branch information
cole-h authored and wes-adams committed Jul 3, 2023
1 parent 604bb23 commit 69a42d5
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 5 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions helix-term/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ grep-searcher = "0.1.11"

[target.'cfg(not(windows))'.dependencies] # https://github.com/vorner/signal-hook/issues/100
signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] }
libc = "0.2.132"

[build-dependencies]
helix-loader = { version = "0.6", path = "../helix-loader" }
Expand Down
32 changes: 27 additions & 5 deletions helix-term/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,7 @@ use anyhow::{Context, Error};

use crossterm::{event::Event as CrosstermEvent, tty::IsTty};
#[cfg(not(windows))]
use {
signal_hook::{consts::signal, low_level},
signal_hook_tokio::Signals,
};
use {signal_hook::consts::signal, signal_hook_tokio::Signals};
#[cfg(windows)]
type Signals = futures_util::stream::Empty<()>;

Expand Down Expand Up @@ -447,7 +444,32 @@ impl Application {
match signal {
signal::SIGTSTP => {
self.restore_term().unwrap();
low_level::emulate_default_handler(signal::SIGTSTP).unwrap();

// SAFETY:
//
// - helix must have permissions to send signals to all processes in its signal
// group, either by already having the requisite permission, or by having the
// user's UID / EUID / SUID match that of the receiving process(es).
let res = unsafe {
// A pid of 0 sends the signal to the entire process group, allowing the user to
// regain control of their terminal if the editor was spawned under another process
// (e.g. when running `git commit`).
//
// We have to send SIGSTOP (not SIGTSTP) to the entire process group, because,
// as mentioned above, the terminal will get stuck if `helix` was spawned from
// an external process and that process waits for `helix` to complete. This may
// be an issue with signal-hook-tokio, but the author of signal-hook believes it
// could be a tokio issue instead:
// https://github.com/vorner/signal-hook/issues/132
libc::kill(0, signal::SIGSTOP)
};

if res != 0 {
let err = std::io::Error::last_os_error();
eprintln!("{}", err);
let res = err.raw_os_error().unwrap_or(1);
std::process::exit(res);
}
}
signal::SIGCONT => {
self.claim_term().await.unwrap();
Expand Down

0 comments on commit 69a42d5

Please sign in to comment.