vty does not restore terminal line setting if process messed with it?

[romanofski]

Background issue with reproducer and detail: https://github.com/purebred-mua/purebred/issues/336

Long story short, when using Brick's suspendAndResume to run vim in a separate process, killing vim leaves the terminal line settings messed up so that the continuing Brick application can't correctly handle input any more. Digging through other applications, a terminal Python library and vty it seems vty/Brick(?) does not restore the terminal line settings before the external program ran, but perhaps should?

I'd be happy to implement a patch, perhaps using a ffi call to tcgetattr during startup? Perhaps termcap could be used for this too?

Let me know whether this all makes sense, since I'm a bit inexperienced with the lingo and may have mixed up terms.

[jtdaugherty]

Interesting. What I can say at this point is that this shouldn't happen, because brick just creates a brand new vty context in suspendAndResume. Here is the relevant code in brick. But that "shouldn't" depends on whether vty is making assumptions about what its terminal setup duties are; maybe killing vim leaves some things set that vty doesn't think to unset. Some relevant code in vty might be here and here.

[romanofski]

Thank you so much for the pointers! I have the suspicion that the new vty context is using the terminal settings vim has set, not what the terminal was originally set to. I will check to verify whether that's true or not, since I don't know the code that well.

[jtdaugherty]

Yeah, I think a challenge is that what the terminal was "originally set to" before the application started really has no relationship to what the terminal state is in when vim stops, from the Vty/Brick perspective. I think the Vty/Brick application is responsible for restoring the state to what it is prior to it taking over the terminal, but if that original state was garbage, it should restore it to garbage.

The issue, to me, would come down to whether the Vty/Brick terminal state configuration is incomplete because it makes assumptions that the starting state will be such-and-such, rather than explicitly setting all of the flags/settings that are needed.

[romanofski]

Hm I had to think about your comment a bit. Just to confirm I understand you correctly: For you "suspending" is equivalent to stopping and starting the application which forks the process. The difference here is just, that during the start, the process didn't clean up the terminal correctly so that the starting Vty/Brick application is using a "messed" up terminal?

So it comes down to whether the initialization of the terminal during the start of a Vty/Brick application does the "right" thing?

[jtdaugherty]

For you "suspending" is equivalent to stopping and starting the application which forks the process.

That's right. If you look at the implementation of suspendAndResume, it calls shutdown on the current vty handle (which restores the terminal state to what it was prior to taking over), invokes the user-supplied IO action (which in Matterhorn's case runs the external editor), and then takes over the terminal again by making a new vty handle with buildVty.

The difference here is just, that during the start, the process didn't clean up the terminal correctly so that the starting Vty/Brick application is using a "messed" up terminal?

Putting it in terms of what I described above, what I'm imagining is that the IO action did something that left the terminal in some undefined state, but that when buildVty ran, it did its usual thing - saving the state of the terminal and setting whatever flags and settings it needed to set. What I suspect is that buildVty doesn't know to do some kinds of setting/unsetting in the face of such corruption because it usually runs from a clean terminal state. I haven't looked into the details of what those settings were (although that might be informed by the stty comparisons in the purebred ticket).

So it comes down to whether the initialization of the terminal during the start of a Vty/Brick application does the "right" thing?

I believe so, but "right" relative to the weird state a killed vim left behind, not just relative to the "normal" terminal state.

[jtdaugherty]

Another option would be to make vty issue the same reset sequences that tput reset (or just reset) does, although I find that when I run reset on the command line it takes about a full second to complete. I'm not sure I like the idea of introducing that delay into the vty startup procedure (or Matterhorn's pre-resume behavior) but I could imagine providing the functionality as a last resort.

Out of curiosity, in your case do you see vim being killed as a normal use case? I would imagine that it is not.

[romanofski]

No it's absolutely not the normal use case and I'm battling a bit with myself whether it's a problematic enough that there should be a solution for it. It does irk me a bit that other mailers handling the possible scenario nicer however.

I agree, I don't think a whole reset is a good way forward. Not that I would know a special case, but I would wonder whether a whole reset would destroy someone's explicitly set attributes.

For what it's worth, I'd be happy to park our Purebred ticket and come back to this problem if there is a less contrived case. That means we close this issue for now.

[jtdaugherty]

It does irk me a bit that other mailers handling the possible scenario nicer however.

That makes sense to me. I don't mean to suggest it isn't worth dealing with this problem better. I only wanted to understand more about the situation.

For what it's worth, I'd be happy to park our Purebred ticket and come back to this problem if there is a less contrived case. That means we close this issue for now.

I'd be happy to dig into this problem more if I could reproduce it, actually. My test that I reported on the purebred ticket was one where I wasn't seeing the behavior, but if I could reproduce, it would give me something to work against. Can you share more about the environment where you're able to get into the broken state? Things like:

• Version of vim
• Terminal emulator in use
• The value of TERM at the time
• The output of stty before and after the problem arises

[jtdaugherty]

the continuing Brick application can't correctly handle input any more

I'm also curious to know a bit more about the problem when it arises. Does the above text mean that the input doesn't arrive to the application? I.e., you press keys, but the application isn't responding?

[romanofski]

I've checked a bit more. I do have vim plugins installed and I have the suspicion that perhaps one of them causes some form of clean up in vim not to run. If I run vim without my configuration file:

vim -u NONE /tmp/foo

and kill this process (simple SIGTERM, the terminal is correctly reset back to:

Vim: Caught deadly signal TERM

Vim: Finished.
Terminated
$ stty
speed 38400 baud; line = 0;

[romanofski]

I'm attaching my vimrc. I can reproduce the terminal changes with this config. Terminal has changed to:

$  vim -u /tmp/vimrc /tmp/foo
Vim: Caught deadly signal TERM

Vim: Finished.
[1]    30409 terminated  vim -u /tmp/vimrc /tmp/foo

$  stty
speed 38400 baud; line = 0;
                           -icrnl
                                 -onlcr
                                       -isig -iexten -echoe

[jtdaugherty]

I don't see any noticeable terminal misbehavior but I do see a difference in stty output with that configuration.

Before running vim:

$ stty
speed 9600 baud;
lflags: echoe echok echoke echoctl
iflags: -ixany ignpar
oflags: -oxtabs
cflags: cs8 -parenb

and after the kill -TERM:

$ stty
speed 9600 baud;
lflags: echoe echok echoke echoctl pendin
iflags: -ixany ignpar
oflags: -oxtabs
cflags: cs8 -parenb

[jtdaugherty]

That pendin flag is awfully interesting because it does relate to input processing, and to canonical mode (which is important to vty).

[jtdaugherty]

On further investigation, it doesn't look like the pendin flag can be changed with stty; it seems to just be reporting on the status of the terminal.

[jtdaugherty]

With that out of the way, that means my stty outputs are otherwise identical.

[jtdaugherty]

I also tried using your vimrc with vim and launching it from matterhorn and killing vim, but Matterhorn's input processing was unaffected after vim terminated.

[jtdaugherty]

(Matterhorn is a brick/vty-based application)

[jtdaugherty]

Oh, actually: matterhorn seemed fine but Enter keypresses were apparently not received. When I exited Matterhorn, the terminal state was just as broken for me as it seems for you.

[jtdaugherty]

I can also confirm that I was able to get into that broken state with my own vimrc, so I don't think your config necessarily includes anything critical to reproducing this.

[romanofski]

Yes the Enter key not working was the initial question mark as to why I was unable to navigate out of Purebred and filed the issue. Interesting to hear that you can reproduce it with your own configuration. I'm now almost wondering whether a specific vim configuration in regards to terminals might cause it.

Btw. in case it matters, I can reproduce the problem on Fedora 31 and NixOS 20.03.2015.e7752db2fb6 (Markhor), yet both have plugins installed.

[kquick]

You might want to use stty -a and check the values for icrnl and inlcr (also ocrnl and onlret for the output behavior above). See https://linux.die.net/man/1/stty.

[kquick]

And these would be controlled in libc via https://linux.die.net/man/3/termios

[kquick]

Testing via matterhorn, matterhorn+emacs works fine, but matterhorn+vi reproduces the issue for me as well. When back in matterhorn, Enter and C-m (carriage return) are ignored, but C-j (newline) works.

Comparing the pre-stty -a output to the post-vi-stty -a output, I can see changes:

   Old/good     | new/corrupted
   -------------|-----------------
   icrnl        | -icrnl
   onlcr        | -onlcr
   isig         | -isig
   icanon       | -icanon
   iexten       | -iexten
   echo         | -echo
   echoe        | -echoe

And just for completeness, this is with VIM - Vi IMproved 8.2 (2019 Dec 12 ...)

[jtdaugherty]

Looks like vty may need to explicitly set some flags. Currently it is only clearing flags, which suggests my hypothesis about this ticket might be right. The flag list is implemented as the TerminalMode type here and the flag clearing (but not setting) is here.

[jtdaugherty]

It also occurs to me that I'll need to think carefully about how Brick's suspendAndResume should handle flag changes because I wouldn't want a program that poisoned the terminal state in a suspendAndResume to cause that poisoning to be carried through to the vty application exit and terminal state restore.

[jtdaugherty]

After playing around with this, setting icrnl at vty initialization time seems to be enough to get the Vty-based application to behave well enough after starting after a killed vim as described in this ticket. I don't fully understand how the behavior of the onlcr flag could or should affect Vty-based applications; my read of the documentation suggests to me that it should not matter what that flag is set to because newlines should not get emitted by Vty due to how they are transformed into Vty-level line breaks. Still, I'd like more clarity if anyone else knows how that should work.

[jtdaugherty]

The patch for this fix is on the bugfix/terminal-init-flags branch for testing.

[romanofski]

\o/ I'll give it a go :)

[jtdaugherty]

(Also, incidentally, as of unix-2.7.2.2, the TerminalMode type doesn't even provide a constructor for onlcr; it's listed as a TODO.)

[romanofski]

Yeah this now behaves much better.

I wonder if I not better follow the rabbit hole down to figure out what's causing vim to leave the terminal in this state. I so far can't put a finger on it what exactly is causing this. I asked on #vim on freenode yesterday and it seems for some it's not simply reproducible. Interestingly enough, even my vim -u NONE invocation now leaves the terminal in a bad state when killed.

[jtdaugherty]

@romanofski Thanks for testing, I'm glad to hear it helps!

As for vim, I think it's really just a matter of vim not resetting the terminal flags back to what they were when it started when a kill signal is received. The flag state it's leaving behind is the state it needed to run, but it's just not a good state for whatever runs next.

[jtdaugherty]

From https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html:

The SIGTERM signal is a generic signal used to cause program termination. Unlike SIGKILL, this signal can be blocked, handled, and ignored. It is the normal way to politely ask a program to terminate.

So it's possible to set up a handler for TERM that restores the terminal state.

[jtdaugherty]

To address the Brick suspendAndResume issue I also added 812e8734999204a5b2b7308c2aaef77a49d5acb7. The fixes on my branch have been merged to master and will be released shortly. I'll follow that with a brick release to make brick applications immune to propagating terminal input state corruption through to their final terminal states. Thanks, everyone!

[jtdaugherty]

This is now released in 5.29.

[jtdaugherty]

Brick 0.55 is now released as well.