Reset terminal at prompt to clean up after bad termination of certain scripts

[mqudsi]

Not sure what exactly is the cause, but many cli apps that use escape codes to present a console ui (not sure if curses only or in general) can crash/exit at a point that will break input, for example, forcing fish_config and descendents to quit while w3m is running results in breakage of the up/down/right/left arrow keys (pressing up results in [A posted to the console, down in [B, etc.)

tput init does not help.

[krader1961]

The example you cite (arrow keys not working) is usually because the app enabled alternate keypad mode (sometimes known as application keypad mode). The tput init command typically includes the sequence \e> to enable numeric keypad mode. See https://vt100.net/docs/vt100-ug/chapter3.html. Whether fish should send the terminfo is or init sequence (which is what tput init emits) before every prompt is debatable. Neither bash or zsh do so. Of course you can argue that fish should do so because ensuring the terminal is in a known state is friendlier but the concern I would have involves possible unwanted side-effects and the cost of including that string with every prompt.

[mqudsi]

Hmmm, tput init didn't help for me in that case (and in the past) with xterm-256color as my TERM with the termcap from ncurses 5.9 (2014-07-26).

The problem is that sometimes (esp. when breaking fish_prompt while testing) ctrl+c, del, and ctrl+h/bkspc becomes uninterpretable so that after a typo there is no option but to kill the process out-of-band (since I can no longer type in exit). Obviously that particular case (fish dev) is an edge case, but other such escaping issues exist.

[krader1961]

@mqudsi, I missed the "does not help" part of your original comment. Nonetheless, what you're describing in your second comment implies that fish is not setting the tty driver mode to a sane value after running an external command. Something that should be impossible (at least as of 2017-09-08 when I stopped tracking the master branch). Also, whatever escape sequences the application has sent to the terminal (e.g., to enable alternate keypad mode) can't possibly affect how fish handles things like [ctrl-C] or [backspace]. What the app does to the tty driver mode can if fish is no longer setting the tty driver mode to a sane value after an external command.

Is this on a UNIX like platform? That is, not Cygwin or WSL? If so execute tty before recreating the problem to capture the tty name. Capture the tty mode by running stty -a < $tty where $tty is the tty name you captured. Then recreate the problem and run, in a different terminal on the same system, stty -a < $tty. Are they the same? They should be identical.

Also, when you say

... so that after a typo there is no option but to kill the process...

it seems to me the need to kill the fish process is irrelevant. The fact it no longer responds to something as basic as the interrupt character (normally [ctrl-c]) is all we need to know. So I'm wondering if I've misunderstood what you're saying.

[zanchey]

Does stty sane help?

[krader1961]

Note that if the problem is such that stty sane fixes it then you'll probably need to press [ctrl-j] rather than [enter] to run the command. And if that does fix the problem it means fish is no longer ensuring the tty modes are sane after an external command is run.

[mqudsi]

@zanchey I'll have to work up a minimal reproduction case so I can test the stty sane approach, but essentially yes, fish is (no longer?) ensuring a sane tty after external commands.

@krader1961 The reason I didn't feel that the lack of response to ctrl+c was enough is because it isn't so much fish ignoring sigint (which I should test via an external kill -SIGINT next time) as it was that ctrl+c was no longer mapped to sigint.

Depending on the repro, sometimes enter still works and sometimes it doesn't (it did this last time, but when I broke fish_prompt it wouldn't any more). I'll try ctrl+j next time that happens, too, thanks for the tip, @krader1961 .

[mqudsi]

I'm able to reproduce this now on kernel vtty (not x11 or ssh).

#include <SDL.h>
int main(int argc, char * argv[]) {
    SDL_Init(SDL_INIT_VIDEO);
    SDL_SetVideoMode(500, 500, 32, SDL_DOUBLEBUF | SDL_OPENGL); // should fail
    return 1; //without calling SDL_Quit();
}

Executing the resulting binary causes the console to act up (input is no longer process correctly) under FreeBSD 12 from the login terminal. Cannot switch sessions, exit fish, or do anything useful.

ssh into the machine and kill the fish login session and everything goes back to normal.

[faho]

@mqudsi: Are you sure that's about terminal modes getting screwed up?

There's a couple more things that could have happened there - did fish actually gain back control? Or did the display get frozen?

I actually think this being a problem with tty modes is fairly unlikely because fish is already resetting them.

[mqudsi]

No, fish gets back control and I can interact with fish, it's just input is scrambled.

[faho]

How is it scrambled? Does pressing ctrl-j or ctrl-m execute stufff? As in, did you try stty sane<ctrl-j>, as was mentioned earlier in this issue?

The weird part here is that fish is already supposed to restore the modes (https://github.com/fish-shell/fish-shell/blob/master/src/reader.cpp#L349-L363)!

Is that not happening? Is there something missing? Is there a race?

[faho]

@mqudsi: Does this still happen for you?

I've not actually ever seen this with fish.

[mqudsi]

I haven't experienced it in the past couple of months, but I haven't been working on anything that manipulates the terminal too much. Note that I already posted test code above which reproduces it 100% of the time, I could try that again if need be.

I don't think it's a race condition because running a subsequent command should trigger the code again and make it work, and that wasn't the case for me.

I imagine there's some state that we aren't saving/restoring? Or maybe it's an issue with the terminal (or terminal emulator) itself?

@krader1961 I'm thinking now that there's no way stty sane could actually ever work because we deliberately break stty, don't we? #2315

[faho]

I imagine there's some state that we aren't saving/restoring?

stty -a before and after should show that.

I'm thinking now that there's no way stty sane could actually ever work because we deliberately break stty, don't we?

We restore the modes. So if we're missing any, stty sane should still affect those.

[faho]

Nobody has experienced this in a couple of months, let's close it for now!

[mqudsi]

Ok, I ran into this using the Elementary OS terminal application (it's its own thing) with whatever version of fish is in the Ubuntu repository used by Elementary OS Hera, but wasn't able to come up with a repro. But this is important because it means that the repro I was able to come up with isn't a bug in the specific terminal I'm using. Read on.

I do, however, have a very convoluted and specific repro that demonstrates the problem every single time. I run neovim under wsl in conhost with termguicolors and a colored theme then send SIGBART to the neovim instance (actually reproducing a crash that triggered this). It crashes leaving the terminal in a broken state: any mouse clicks put garbage to the screen. stty sane, reset, and tput init do nothing to fix it. stty reports nothing odd (which makes sense, or else stty sane would have fixed it).

I was leaning heavily towards this being a bug in conhost, but then I discovered that merely starting fzf then stopping it (via ^C) makes the terminal work again; in fact, starting many different ncurses applications and then exiting them makes the terminal work (both nvim and fzf, for example, but importantly, not fish).

I believe that in addition to resetting the terminal state we also need to reset something in ncurses: all of fish, nvim, and fzf use ncurses and when I send neovim SIGBART, I rob it of the opportunity to call endwin(). It is obvious that there's something that fzf and neovim do in their startup or tear-down code that correctly resets something in the terminal environment via ncurses, but fish does not do this: as a result, starting and stopping fzf or nvim after the bug causes the terminal to be returned to a sane state, but starting and stopping fish itself doesn't. This is likely the same thing we need to do when control of the terminal is returned from an application.

[faho]

Okay I'm assuming this is either #7133 or d5a239e.

[mqudsi]

@faho can you please dial back on closing issues? It makes it hard to keep track of things that haven't been tested and confirmed fixed/wontfix.

(and this particular issue still replicates; #7133 mentions that it's a separate bug from this one, and d5a239e has nothing to do with this, it was only termsize)

[mqudsi]

This is quite possibly a separate issue but with the same underlying cause (ncurses modifies something in the terminal state that fish does not restore):

After ctrl-x,ctrl-a in gdb (show split screen source code), followed by info thread with a lot of threads (point is to trigger paging), then ctrl-x,ctrl-a again to remove split screen, bottom pane contents (historical + any future commands) are no longer aligned (reflow is broken in gdb) - up to this point, it's all GDB misbehaving. However, exit gdb now (cleanly) via ctrl-d,y,<enter> and fish is left in a broken state:

1 mqudsi@Blitzkrieg /m/c/U/Mahmoud> stty
speed 38400 baud; line = 0;
                           eol = M-^?; eol2 = M-^?; swtch = M-^?;
                                                                 ixany iutf8
                                                                            -onlcr
                                                                                  ¶                                                                                         
1 mqudsi@Blitzkrieg /m/c/U/Mahmoud> stty -a
speed 38400 baud; rows 97; columns 172; line = 0;
                                                 intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = M-^?; eol2 = M-^?; swtch = M-^?; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V;
                                         discard = ^O; min = 1; time = 0;
                                                                         -parenb -parodd -cmspar cs8 hupcl -cstopb cread -clocal -crtscts
                                                                                                                                         -ignbrk brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl -ixon -ixoff -iuclc ixany imaxbel iutf8
                                                                      opost -olcuc -ocrnl -onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
                                                                                                                                                      isig icanon iexten echo echoe echok -echonl -noflsh -xcase -tostop -echoprt echoctl echoke -flusho -extproc
                                                                                     ¶  

While starting and cleanly exiting an ncurses app (nvim) does not fix the terminal (hypothesis: ncurses correctly restores the previous - albeit broken - terminal state on exit), importantly exec fish did fix the terminal.

[faho]

So, comparing your stty -a output to good output on my machine, the difference in terminal modes is (bad == - to good == +):

-hupcl
+-hupcl
-brkint
+-brkint
-ixany
-imaxbel
+-ixany
+-imaxbel
--onlcr
+onlcr

of which the only output related one is onlcr - "translate newline to carriage return-newline".

And when I do turn that off I indeed get weird staircase output.

So it seems we just need to restore that one? Note that we used to disable it because of #4505. It seems we may need to force it to on instead.

[mqudsi]

I think so?

(I re-opened because the issue described with neovim is different)

[faho]

(I re-opened because the issue described with neovim is different)

Okay, if I understand correctly your issue is:

any mouse clicks put garbage to the screen

Garbage like

?

If so, neovim turned mouse reporting on and didn't turn it off. Fish does not do any mouse handling by itself, and won't currently turn mouse reporting on or off.

The solution is echo -e "\e[?1000l" (echo -e "\e[?1000h" turns it on), but I don't know when that's accepted, I'm not particularly in the mood to do yet more terminal feature detection (can we get a better replacement for terminfo already), and this doesn't seem to be a common problem so I don't feel like handling it is worth it. If neovim crashed, just start neovim again and exit it cleanly, or start a new tab.

[mqudsi]

I don't think this needs any terminal feature detection? If fish doesn't use mouse detection, fish can (should?) just keep it off and any application that needs application-level mouse detection will be turn it back on as needed. I can't think of any apps that act differently (intentionally) based on whether or not the invoking shell has enabled mouse support. It would just have to be one of those states that is saved alongside a background process' state so that it is restored along with it but otherwise disabled.

[faho]

f fish doesn't use mouse detection, fish can (should?) just keep it off and any application that needs application-level mouse detection will be turn it back on as needed

You misunderstand. Fish doesn't need it, but neovim turned it on, and because it crashed did not turn it off!

And because turning it off involves sending an escape sequence, it needs feature detection.

[faho]

Okay, the original issue was fixed, so I am closing this. As for the secondary issue with the mouse reporting, I'll refer to #4918. Turning mouse reporting off requires detecting if mouse reporting is supported in the first place, which is only really something that makes sense in the context of adding support for mouse reporting.

Any other ways to break the terminal (e.g. iTerm might be confused about which host the user is currently operating on) also require fish to know about these escapes. They should also be handled in separate issues.

[mqudsi]

All issues in this thread have now been resolved, with the mouse-related issues being addressed in mqudsi@eecc223 and subsequent commits.