kitty keyboard protocol's "disambiguate escape codes" disables VINTR/VSTOP

[krobelus]

The disambiguate flag 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.
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.

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

One approach is to add a separate thread for handling input, then we can process keys while the main thread is busy.

[zanchey]

Wow

[mqudsi]

That's very unfortunate. You'd think the importance of ctrl-c and the dearth of applications that would want to map it to anything but sigint would be enough reason to provide an escape hatch to mean "disambiguate all but ctrl-c" or similar.

[krobelus]

would be enough reason to provide an escape hatch to mean "disambiguate all but ctrl-c" or similar.

We could definitely extend the protocol¹, see kovidgoyal/kitty#7961 (comment)
it's just that no one has asked for it.

Alternatively, we can process keys from stdin in ctx.check_cancel()
(we already have the functionality to store keys in a queue if we don't want to handle them yet).

Interestingly, nushell also can't ctrl-c infinite loops, but not just for this reason.

Footnotes

1. To unbreak flow control, extending the kitty keyboard protocol is the only option ↩

[faho]

Okay, what do we want to do here for 4.0?

These appear to be the possibilities:

1. Leave everything as-is, kitty protocol users can't cancel the shell itself (with exceptions like wildcards)
2. Turn off kitty keyboard protocol for now (by default?), kitty protocol users can't do fancy key combos
3. Find some solution to read ctrl-c while the shell is otherwise occupied

The difference between 1 and 2 is a trade-off - is it more important that you be able to cancel, or be able to bind ctrl-i separate from tab?

I don't know how exactly 3 would work - can we just read in a background thread and then signal the cancellation checker, or do we read anywhere and would then steal input out from under ourselves? Is this a problem while we're running a builtin?

Personally, unless we get 3 to work, I would lean towards 2 because not breaking cancelling seems more important than adding a feature that didn't exist before.

---

Frankly, I think this is a deficiency in the kitty protocol - it means that, even with the restricted mode, if fish crashes anything else won't be cancellable either unless it handles the kitty protocol.

[zanchey]

Personally, unless we get 3 to work, I would lean towards 2 because not breaking cancelling seems more important than adding a feature that didn't exist before.

I agree, and my concern is that 3 may involve a fairly large change at this stage (but the extended keyboard stuff is a great feature so it might be worth giving it a shot).

[krobelus]

It's almost easy to simply poll stdin for ctrl-c while we are executing builtin commands etc.
The problem is that at this time we are in ICANON, which implies line buffering.

I think we should instead turn off the disambiguate flag while we're executing any command, not just for external ones.
That's simpler overall, and it's also what we used to do (see #10494).
If we overflow our budget of the number of bytes we want print, there are ways to optimize that.

[krobelus]

I'm leaning towards this one. I agree that we should probably extend the kitty keyboard protocol, to make this easier for our use case.

From a00ccfa7b725c72121fc8bfc6dd644c3918535d8 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sat, 23 Nov 2024 17:43:19 +0100
Subject: [PATCH] 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[^1].

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 things when absolutely necessary.

Correctness is more important, so disable things in eval_node() again.
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; open_cloexec()
and fire_delayed(). The first one is not relevant for interactive
sessions and fire_delayed() will end up calling eval_node() where we
already disable it.

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 need to turn off the disambiguate
flag.

Since this is quite change is quite complicated, I plan to extend the
kitty keyboard protocol to optionally support VINTR, VSTOP and friends.
Then we can drop most of these changes.

[^1]: It would be interesting to experiment changing ICANON in future,
but it would surely break lots of workflows (case in point: I'm very
bummed that direnv disables buffering).

Closes #10864
---
 src/builtins/read.rs | 2 ++
 src/builtins/wait.rs | 2 --
 src/exec.rs          | 4 ----
 src/parser.rs        | 3 +++
 src/reader.rs        | 4 ++++
 src/wildcard.rs      | 5 -----
 6 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/src/builtins/read.rs b/src/builtins/read.rs
index 374327f69..7051897f3 100644
--- a/src/builtins/read.rs
+++ b/src/builtins/read.rs
@@ -12,6 +12,7 @@ use crate::env::EnvMode;
 use crate::env::Environment;
 use crate::env::READ_BYTE_LIMIT;
 use crate::env::{EnvVar, EnvVarFlags};
+use crate::input_common::terminal_protocols_disable_ifn;
 use crate::libc::MB_CUR_MAX;
 use crate::nix::isatty;
 use crate::reader::commandline_set_buffer;
@@ -243,6 +244,7 @@ fn read_interactive(
 
         reader_readline(parser, nchars)
     };
+    terminal_protocols_disable_ifn();
     if let Some(line) = mline {
         *buff = line;
         if nchars > 0 && nchars < buff.len() {
diff --git a/src/builtins/wait.rs b/src/builtins/wait.rs
index f49233259..5865317ec 100644
--- a/src/builtins/wait.rs
+++ b/src/builtins/wait.rs
@@ -165,8 +165,6 @@ pub fn wait(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Opt
         return STATUS_CMD_OK;
     }
 
-    crate::input_common::terminal_protocols_disable_ifn();
-
     if w.wopt_index == argc {
         // No jobs specified.
         // Note this may succeed with an empty wait list.
diff --git a/src/exec.rs b/src/exec.rs
index 1e60e539f..299fcb73e 100644
--- a/src/exec.rs
+++ b/src/exec.rs
@@ -72,10 +72,6 @@ pub fn exec_job(parser: &Parser, job: &Job, block_io: IoChain) -> bool {
         return true;
     }
 
-    if job.entitled_to_terminal() {
-        crate::input_common::terminal_protocols_disable_ifn();
-    }
-
     // Handle an exec call.
     if job.processes()[0].typ == ProcessType::exec {
         // If we are interactive, perhaps disallow exec if there are background jobs.
diff --git a/src/parser.rs b/src/parser.rs
index fb62fe0f8..2523f6a03 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -14,6 +14,7 @@ use crate::expand::{
 };
 use crate::fds::{open_dir, BEST_O_SEARCH};
 use crate::global_safety::RelaxedAtomicBool;
+use crate::input_common::terminal_protocols_disable_ifn;
 use crate::io::IoChain;
 use crate::job_group::MaybeJobId;
 use crate::operation_context::{OperationContext, EXPANSION_LIMIT_DEFAULT};
@@ -610,6 +611,8 @@ impl Parser {
         let mut execution_context =
             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.
         let prev_exec_count = self.libdata().exec_count;
         let prev_status_count = self.libdata().status_count;
diff --git a/src/reader.rs b/src/reader.rs
index dee859c4e..7203f388d 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -75,6 +75,7 @@ use crate::history::{
     SearchType,
 };
 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::{
     terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, CharInputStyle, InputData,
@@ -5698,6 +5699,9 @@ impl<'a> Reader<'a> {
         token_range.start += 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
         // wildcard; if that succeeds we don't then apply user completions (#8593).
         let mut wc_expanded = WString::new();
diff --git a/src/wildcard.rs b/src/wildcard.rs
index eeb840cf8..0f6a86a47 100644
--- a/src/wildcard.rs
+++ b/src/wildcard.rs
@@ -439,7 +439,6 @@ mod expander {
     use crate::{
         common::scoped_push,
         path::append_path_component,
-        threads::is_main_thread,
         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
                 let Ok(mut dir) = self.open_dir(
                     base_dir, /* return . and .. */
-- 
2.47.0.318.g04eaff62f2

[krobelus]

actually we can probably delay term_donate() (which sets ICANON) until we actually run an external process.
I expect that the only other place where we need to donate the terminal is in builtin fg.