Disable terminal protocols before cancellable operations

The [disambiguate flag](https://sw.kovidgoyal.net/kitty/keyboard-protocol/#disambiguate) means that:

> In particular, ctrl+c will no longer generate the SIGINT signal,
> but instead be delivered as a CSI u escape code.

so cancellation only works while we turn off disambiguation.

Today we turn it off while running external commands that want to
claim the TTY.  Also we do it (only as a workaround for this issue)
while expanding wildcards or while running builtin wait.

However there are other cases where we don't have a workaround,
like in trivial infinite loops or when opening a fifo.

Before we run "while true; end", we put the terminal back in ICANON
mode. This means it's line-buffered, so we won't be able to detect
if the user pressed ctrl-c.

Commit 8164855b7 (Disable terminal protocols throughout evaluation,
2024-04-02) had the right solution: simply disable terminal protocols
whenever we do computations that might take a long time.
eval_node() covers most of that; there are a few others.

As pointed out in #10494, the logic was fairly unsophisticated then:
it toggled terminal protocols many times.  The fix in 29f2da8d1
(Toggle terminal protocols lazily, 2024-05-16) went to the extreme
other end of only toggling protocols when absolutely necessary.

Back out part of that commit by toggling in eval_node() again,
fixing cancellation.  Fortunately, we can keep most of the benefits
of the lazy approach from 29f2da8d1: we toggle only 2 times instead
of 8 times for an empty prompt.

There are only two places left where we call signal_check_cancel()
without necessarily disabling the disambiguate flag
1. open_cloexec() we assume that the files we open outside eval_node()
   are never blocking fifos.
2. fire_delayed(). Judging by commit history, this check is not
   relevant for interactive sessions; we'll soon end up calling
   eval_node() anyway.

In future, we can leave bracketed paste, modifyOtherKeys and
application keypad mode turned on again, until we actually run an
external command.  We really only want to turn off the disambiguate
flag.

Since this is approach is overly complex, I plan to go with either
of these two alternatives in future:
- extend the kitty keyboard protocol to optionally support VINTR,
  VSTOP and friends.  Then we can drop most of these changes.
- poll stdin for ctrl-c. This promises a great simplification,
  because it implies that terminal ownership (term_steal/term_donate)
  will be perfectly synced with us enabling kitty keyboard protocol.
  This is because polling requires us to turn off ICANON.
  I started working on this change; I'm convinced it must work,
  but it's not finished yet. Note that this will also want to
  add stdin polling to builtin wait.

Closes #10864
This commit is contained in:
Johannes Altmanninger 2024-11-23 17:43:19 +01:00
parent f0a5f8738b
commit b89619330b
8 changed files with 12 additions and 14 deletions

View file

@ -12,6 +12,7 @@ use crate::env::EnvMode;
use crate::env::Environment; use crate::env::Environment;
use crate::env::READ_BYTE_LIMIT; use crate::env::READ_BYTE_LIMIT;
use crate::env::{EnvVar, EnvVarFlags}; use crate::env::{EnvVar, EnvVarFlags};
use crate::input_common::terminal_protocols_disable_ifn;
use crate::libc::MB_CUR_MAX; use crate::libc::MB_CUR_MAX;
use crate::nix::isatty; use crate::nix::isatty;
use crate::reader::commandline_set_buffer; use crate::reader::commandline_set_buffer;
@ -243,6 +244,7 @@ fn read_interactive(
reader_readline(parser, nchars) reader_readline(parser, nchars)
}; };
terminal_protocols_disable_ifn();
if let Some(line) = mline { if let Some(line) = mline {
*buff = line; *buff = line;
if nchars > 0 && nchars < buff.len() { if nchars > 0 && nchars < buff.len() {

View file

@ -165,8 +165,6 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
return STATUS_CMD_OK; return STATUS_CMD_OK;
} }
crate::input_common::terminal_protocols_disable_ifn();
if w.wopt_index == argc { if w.wopt_index == argc {
// No jobs specified. // No jobs specified.
// Note this may succeed with an empty wait list. // Note this may succeed with an empty wait list.

View file

@ -72,10 +72,6 @@ pub fn exec_job(parser: &Parser, job: &Job, block_io: IoChain) -> bool {
return true; return true;
} }
if job.entitled_to_terminal() {
crate::input_common::terminal_protocols_disable_ifn();
}
// Handle an exec call. // Handle an exec call.
if job.processes()[0].typ == ProcessType::exec { if job.processes()[0].typ == ProcessType::exec {
// If we are interactive, perhaps disallow exec if there are background jobs. // If we are interactive, perhaps disallow exec if there are background jobs.

View file

@ -14,6 +14,7 @@ use crate::expand::{
}; };
use crate::fds::{open_dir, BEST_O_SEARCH}; use crate::fds::{open_dir, BEST_O_SEARCH};
use crate::global_safety::RelaxedAtomicBool; use crate::global_safety::RelaxedAtomicBool;
use crate::input_common::terminal_protocols_disable_ifn;
use crate::io::IoChain; use crate::io::IoChain;
use crate::job_group::MaybeJobId; use crate::job_group::MaybeJobId;
use crate::operation_context::{OperationContext, EXPANSION_LIMIT_DEFAULT}; use crate::operation_context::{OperationContext, EXPANSION_LIMIT_DEFAULT};
@ -610,6 +611,8 @@ impl Parser {
let mut execution_context = let mut execution_context =
ExecutionContext::new(ps.clone(), block_io.clone(), Rc::clone(&line_counter)); ExecutionContext::new(ps.clone(), block_io.clone(), Rc::clone(&line_counter));
terminal_protocols_disable_ifn();
// Check the exec count so we know if anything got executed. // Check the exec count so we know if anything got executed.
let prev_exec_count = self.libdata().exec_count; let prev_exec_count = self.libdata().exec_count;
let prev_status_count = self.libdata().status_count; let prev_status_count = self.libdata().status_count;

View file

@ -75,6 +75,7 @@ use crate::history::{
SearchType, SearchType,
}; };
use crate::input::init_input; use crate::input::init_input;
use crate::input_common::terminal_protocols_disable_ifn;
use crate::input_common::IN_MIDNIGHT_COMMANDER_PRE_CSI_U; use crate::input_common::IN_MIDNIGHT_COMMANDER_PRE_CSI_U;
use crate::input_common::{ use crate::input_common::{
terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, CharInputStyle, InputData, terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, CharInputStyle, InputData,
@ -5689,6 +5690,9 @@ impl<'a> Reader<'a> {
token_range.start += cmdsub_range.start; token_range.start += cmdsub_range.start;
token_range.end += cmdsub_range.start; token_range.end += cmdsub_range.start;
// Wildcard expansion and completion below check for cancellation.
terminal_protocols_disable_ifn();
// Check if we have a wildcard within this string; if so we first attempt to expand the // Check if we have a wildcard within this string; if so we first attempt to expand the
// wildcard; if that succeeds we don't then apply user completions (#8593). // wildcard; if that succeeds we don't then apply user completions (#8593).
let mut wc_expanded = WString::new(); let mut wc_expanded = WString::new();

View file

@ -439,7 +439,6 @@ mod expander {
use crate::{ use crate::{
common::scoped_push, common::scoped_push,
path::append_path_component, path::append_path_component,
threads::is_main_thread,
wutil::{dir_iter::DirIter, normalize_path, DevInode}, wutil::{dir_iter::DirIter, normalize_path, DevInode},
}; };
@ -583,10 +582,6 @@ mod expander {
} }
} }
if is_main_thread() {
crate::input_common::terminal_protocols_disable_ifn();
}
// return "." and ".." entries if we're doing completions // return "." and ".." entries if we're doing completions
let Ok(mut dir) = self.open_dir( let Ok(mut dir) = self.open_dir(
base_dir, /* return . and .. */ base_dir, /* return . and .. */

View file

@ -56,12 +56,12 @@ print_var_contents("foo", "bar")
# read -c (see #8633) # read -c (see #8633)
sendline(r"read -c init_text somevar && echo $somevar") sendline(r"read -c init_text somevar && echo $somevar")
expect_re(r"\r\n?read> init_text$") expect_re(r"\r\n?.*read> init_text")
sendline("someval") sendline("someval")
expect_prompt("someval\r\n") expect_prompt("someval\r\n")
sendline(r"read --command='some other text' somevar && echo $somevar") sendline(r"read --command='some other text' somevar && echo $somevar")
expect_re(r"\r\n?read> some other text$") expect_re(r"\r\n?.*read> some other text")
sendline("another value") sendline("another value")
expect_prompt("another value\r\n") expect_prompt("another value\r\n")

View file

@ -48,7 +48,7 @@ expect_prompt()
sendline("function postexec --on-event fish_postexec; echo fish_postexec spotted; end") sendline("function postexec --on-event fish_postexec; echo fish_postexec spotted; end")
expect_prompt() expect_prompt()
sendline("read") sendline("read")
expect_re(r"\r\n?read> $", timeout=10) expect_re(r"\r\n?.*read> .*", timeout=10)
sleep(0.1) sleep(0.1)
os.kill(sp.spawn.pid, signal.SIGINT) os.kill(sp.spawn.pid, signal.SIGINT)
expect_str("fish_postexec spotted", timeout=10) expect_str("fish_postexec spotted", timeout=10)