helix icon indicating copy to clipboard operation
helix copied to clipboard

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

Open cole-h opened this issue 3 years ago • 11 comments

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 https://github.com/helix-editor/helix/issues/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 avatar Aug 25 '22 19:08 cole-h

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

cole-h avatar Aug 25 '22 19:08 cole-h

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.

the-mikedavis avatar Aug 25 '22 20:08 the-mikedavis

Latest commit switches from nix to libc.

cole-h avatar Aug 25 '22 20:08 cole-h

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

kirawi avatar Aug 25 '22 21:08 kirawi

See https://github.com/helix-editor/helix/pull/1843 and https://github.com/vorner/signal-hook/issues/132 we were hoping to fix this in signal-hook instead

archseer avatar Aug 26 '22 05:08 archseer

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.

cole-h avatar Aug 26 '22 17:08 cole-h

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

archseer avatar Aug 30 '22 01:08 archseer

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.

cole-h avatar Aug 30 '22 01:08 cole-h

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.

pickfire avatar Aug 30 '22 15:08 pickfire

Rebased.

cole-h avatar Oct 06 '22 23:10 cole-h

Before merging this, I think we need an investigation in https://github.com/vorner/signal-hook/issues/132#issuecomment-1079897156, 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.

pickfire avatar Oct 07 '22 00:10 pickfire

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()
}

pickfire avatar Jan 21 '23 05:01 pickfire