Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Mar 13, 2023

Conversation

cole-h
Copy link
Contributor

@cole-h cole-h commented Aug 25, 2022

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.


Fixes #3522.


I opted to add the nix crate because it has nice Rust wrappers around e.g. kill, handling error cases and other stuff for us. However, I could easily port it to the libc crate directly, if desired.

@cole-h
Copy link
Contributor Author

cole-h commented Aug 25, 2022

I tested this with:

$ cargo b
$ VISUAL=$PWD/target/debug/hx git commit --allow-empty
# ^Z
$ echo "tada I'm back at my terminal"
tada I'm back at my terminal
$ fg
# finish git commit

@the-mikedavis
Copy link
Member

Yeah I think it would be preferrable to use libc directly. Pulling in nix for a handful of small functions doesn't seem worth it.

@cole-h
Copy link
Contributor Author

cole-h commented Aug 25, 2022

Latest commit switches from nix to libc.

@kirawi
Copy link
Member

kirawi commented Aug 25, 2022

It might be worth adding a comment explaining why it's safe.

@archseer
Copy link
Member

See #1843 and vorner/signal-hook#132 we were hoping to fix this in signal-hook instead

@cole-h
Copy link
Contributor Author

cole-h commented Aug 26, 2022

I'll gladly revert this once that does get handled upstream, but seeing the upstream issue closed with no further activity does not inspire confidence it will be fixed anytime soon. This also resolves one of my longstanding annoyances when using helix is my general purpose editor, so I would really love if we could accept this as "good for now" until signal-hook decides to fix this case.

@archseer
Copy link
Member

Maybe you could just upstream the fix? @pickfire closed the issue since he couldn't track it down.

@cole-h
Copy link
Contributor Author

cole-h commented Aug 30, 2022

Unfortunately, I don't have the time or energy to attempt to design this in a way upstream might consider palatable at the moment. Sorry.

@pickfire
Copy link
Contributor

pickfire commented Aug 30, 2022

I closed the issue previously because I feel like it's a hack. I was thinking of checking this with tokio since I think the issue originates there (or maybe it really is signal-hook) but I don't have the energy to check with them back then.

But thinking about it now again I still think it is good to have this, quite a few times I wanted to reopen my previous pull request.

Comment on lines +400 to +472
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);
}
Copy link
Contributor

@pickfire pickfire Aug 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do this previously because I don't know when will this happen, so I just drop in an unwrap.

@kirawi kirawi added C-bug Category: This is a bug A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@cole-h
Copy link
Contributor Author

cole-h commented Oct 6, 2022

Rebased.

@pickfire
Copy link
Contributor

pickfire commented Oct 7, 2022

Before merging this, I think we need an investigation in vorner/signal-hook#132 (comment), as well as looking to see if the issue lies somewhere within tokio. Both @cole-h and I probably won't be able to it at this moment of time.

@archseer archseer added this to the 22.11 milestone Nov 17, 2022
@archseer archseer removed this from the 22.11 milestone Nov 29, 2022
@cole-h cole-h force-pushed the fix-ctrl-z-handling branch 2 times, most recently from c88bca9 to 406fd0f Compare December 7, 2022 18:11
@pickfire
Copy link
Contributor

pickfire commented Jan 21, 2023

Not sure why but when I tested with crossterm example again, it seemed to work.

I cloned crossterm and modified the example event-stream-tokio.

code
//! Demonstrates how to read events asynchronously with tokio.
//!
//! cargo run --features="event-stream" --example event-stream-tokio
//!
//! Add to dev-dependencies
//!
//! signal-hook-tokio = { version = "0.3", features = ["futures-v0_3"] }

use libc::c_int;
use signal_hook::consts::signal::*;
use signal_hook::low_level;
use signal_hook_tokio::Signals;
use std::{io::stdout, time::Duration};

use futures::{future::FutureExt, select, StreamExt};
use futures_timer::Delay;

use crossterm::{
    cursor::position,
    event::{
        DisableMouseCapture, EnableMouseCapture, Event, EventStream, KeyCode, KeyEvent,
        KeyModifiers,
    },
    execute,
    terminal::{disable_raw_mode, enable_raw_mode},
    Result,
};

const HELP: &str = r#"EventStream based on futures_util::Stream with tokio
 - Keyboard, mouse and terminal resize events enabled
 - Prints "." every second if there's no event
 - Hit "c" to print current cursor position
 - Use Esc to quit
"#;

async fn print_events() {
    const SIGNALS: &[c_int] = &[
        SIGTERM, SIGQUIT, SIGINT, SIGTSTP, SIGWINCH, SIGHUP, SIGCHLD, SIGCONT,
    ];
    let mut reader = EventStream::new();
    let mut sigs = Signals::new(SIGNALS).unwrap();

    loop {
        let mut delay = Delay::new(Duration::from_millis(1_000)).fuse();
        let mut event = reader.next().fuse();

        select! {
            _ = delay => { println!(".\r"); },
            maybe_event = event => {
                match maybe_event {
                    Some(Ok(event)) => {
                        println!("Event::{:?}\r", event);

                        if event == Event::Key(KeyCode::Char('c').into()) {
                            println!("Cursor position: {:?}\r", position());
                        }

                        if event == Event::Key(KeyCode::Esc.into()) {
                            break;
                        }

                        if matches!(event, Event::Key(KeyEvent {
                            code: KeyCode::Char('z'),
                            modifiers: KeyModifiers::CONTROL,
                            ..
                        })) {
                            low_level::raise(SIGTSTP).unwrap();
                        }
                    }
                    Some(Err(e)) => println!("Error: {:?}\r", e),
                    None => break,
                }
            }
            maybe_signal = sigs.next().fuse() => {
                if let Some(signal) = maybe_signal {
                    println!("Received signal {:?}\r", signal);
                    // After printing it, do whatever the signal was supposed to do in the first place
                    if let SIGTSTP = signal {
                        disable_raw_mode().unwrap();
                        low_level::emulate_default_handler(signal).unwrap();
                        enable_raw_mode().unwrap();
                    } else {
                        low_level::emulate_default_handler(signal).unwrap();
                    }
                }
            }
        };
    }
}

#[tokio::main]
async fn main() -> Result<()> {
    println!("{}", HELP);

    enable_raw_mode()?;

    let mut stdout = stdout();
    execute!(stdout, EnableMouseCapture)?;

    print_events().await;

    execute!(stdout, DisableMouseCapture)?;

    disable_raw_mode()
}

@archseer archseer added this to the next milestone Feb 11, 2023
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.
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).
Also add a link back to one of the upstream issues.
Copy link
Contributor

@pickfire pickfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and well documented. Thanks for working on this and keeping it up to date.

Since we don't really know for a good non-hacky way to fix this just yet, we can go with this for now since it solves the issue too.

@pickfire pickfire removed the S-waiting-on-review Status: Awaiting review from a maintainer. label Mar 13, 2023
@pickfire pickfire merged commit 3493473 into helix-editor:master Mar 13, 2023
@cole-h cole-h deleted the fix-ctrl-z-handling branch March 13, 2023 13:43
sagnibak pushed a commit to sagnibak/helix that referenced this pull request Mar 21, 2023
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.
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

To suspend helix while in the git commit editor, you have to press ^Z twice
5 participants