Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`connmanctl` phantom input if interrupted with ctrl-c #706

Closed
kwshi opened this issue Jun 25, 2018 · 10 comments
Closed

`connmanctl` phantom input if interrupted with ctrl-c #706

kwshi opened this issue Jun 25, 2018 · 10 comments
Labels
bug

Comments

@kwshi
Copy link
Contributor

@kwshi kwshi commented Jun 25, 2018

If connmanctl is started, then killed with Ctrl-C, then started again, input keystrokes are not rendered to the screen---though still somehow registered and processed, a la password input.

More precisely, here's what happens:

  • connmanctl: behaves normally, registering input as expected
  • ctrl-c to interrupt
  • connmanctl again, and typing some keys: none of the keys are rendered to the screen, but they are registered "internally" correctly since pressing return sends an actual command and causes connmanctl to respond with the corresponding output.

This behavior is not exhibited when connmanctl is launched from bash.

Perhaps there is something off about how connmanctl handles stdin, and that elvish is not prepared for it? Other interactive tools that capture stdin, such as cat (I can't think of anything else to test with, for now) also do not exhibit this behavior, suggesting that the issue is particular to connmanctl, or whatever it uses to handle stdin.

@xiaq xiaq added bug comp:ui labels Jul 2, 2018
@xiaq
Copy link
Member

@xiaq xiaq commented Jul 2, 2018

Connmanctl seems to be an Archlinux-specific tool so I cannot reproduce this myself. Can you give me the following information:

  • After the bug is triggered, does other commands like cat behave correctly?
  • What is the output of stty on a "clean" terminal versus after the bug is triggerd?

My current suspicion is that connmanctl left the terminal in a weird state.

@kwshi
Copy link
Contributor Author

@kwshi kwshi commented Jul 3, 2018

cat works correctly. I can't think of other stdin-ish tools to test this behavior with. I'll try the stty as soon as I get back to my laptop.

The error does seem particular to connmanctl, so it's probably true that connmanctl particular does something weird to the terminal. It seems that other shells seem able to cope with this weirdness, though.

P.S. What linux do you use?

@occivink
Copy link

@occivink occivink commented Jul 3, 2018

@xiaq I don't think connmanctl is archlinux-specific, it's a CLI for the connman connection manager.

@kwshi
Copy link
Contributor Author

@kwshi kwshi commented Jul 3, 2018

@xiaq Okay, here is what happens with stty.

In Elvish:

~> stty
speed 38400 baud; line = 0;
-brkint -imaxbel iutf8
~> connmanctl
connmanctl> Exception: connmanctl killed by signal interrupt
[tty], line 1: connmanctl
~> stty
speed 38400 baud; line = 0;
lnext = <undef>; min = 1; time = 0;
-brkint -icrnl -imaxbel iutf8
-icanon -echo

In Bash:

[kshi@dexi ~]$ stty
speed 38400 baud; line = 0;
-brkint -imaxbel iutf8
[kshi@dexi ~]$ connmanctl
connmanctl> 
[kshi@dexi ~]$ stty
speed 38400 baud; line = 0;
-brkint -imaxbel iutf8

Ah, so I've noticed that various other utilities that rely on stdin also break as soon as the connmanctl-interrupt "pollutes" the terminal (they themselves don't seem to cause the polluting, though it's a little hard to tell because everything I've tried, i.e. python, bash, ipython, catch the interrupt themselves).

@xiaq
Copy link
Member

@xiaq xiaq commented Jul 8, 2018

OK, this now confirms my suspicion that connmanctl is putting the terminal in non-canonical mode and it does not restore the terminal when being interrupted. This seems to be a bug of connmanctl but Elvish can do something to mitigate such situations.

Right now, Elvish saves the terminal attributes every time it activates the line editor and restores the attributes when it deactivates the line editor (e.g. to run a command):

restoreTerminal, err := tty.Setup(ed.in, ed.out)

errRestore := ed.restoreTerminal()

This means that when connmanctl is interrupted, leaving the terminal in non-canonical mode, Elvish remembers that state, and puts the terminal back in non-canonical mode when running the next command.

We can change this logic to remember the terminal state only once when Elvish starts, and reuse that terminal state every time.

@xiaq xiaq closed this in 99687f3 Jul 8, 2018
@xiaq
Copy link
Member

@xiaq xiaq commented Jul 8, 2018

This should be fixed now. @kwshi, reopen if you can still reproduce this.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Jul 8, 2018

Note that Elvish has to assume the tty is in a weird state after resuming control from any external command. Even a well behaved command that restored the tty state might be unable to do so because it died from calling abort() or receiving SIGSEGV or SIGKILL. Also keep in mind that the user might want to use the stty command to change tty attributes such as the interrupt key. That is pretty rare these days but not unheard of. See, for example, this fish shell issue.

@xiaq
Copy link
Member

@xiaq xiaq commented Jul 8, 2018

@krader1961 Good point about having well-behaved command leaving the terminal in a bad state.

For the second point, do you mean that Elvish should still make it possible for the user to use stty to control terminal attributes?

It seems to me that 1) guarding against ill-behaving command messing up with the terminal and 2) supporting use of stty are two conflict goals. I don't think you can really distinguish commands messing with the terminal by accident and commands messing with the terminal on purpose. We either respect what the command does to the terminal, and we lose 1; or we ignore what the command does to the terminal and always restores the terminal to a state we know is good, and we lose 2.

There may be a middle ground, namely respecting parts of the terminal setting external commands leaves and restore the others. But that would be pretty inconsistent - for instance, the user might wonder why stty flagx works but stty flagb does not.

Another interesting option is to provide a command that explicitly signals "run this command, and retain the terminal settings it make" to make stty still work to some extent.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Jul 8, 2018

The fish shell issue I linked to has two main schools of thought. I have seen people propose that fish should assume the tty is in a sane state and if it isn't require the user to type things like stty sane [ctrl-j]. Even for a grey beard like myself who was accustomed to typing that at least once a week back in the 1990's that proposal is absurd. The more sensible approaches are:

  1. Use a whitelist of tty attributes the user can change. Things like the magic chars like stty intr (the c_cc[VINTR] termios structure member) would be whitelisted.

  2. Use a blacklist of tty attributes it never makes sense for the user to change. This would include things like the ICANON flag.

Option 2 gives the user maximal control but also makes it more likely that a program that fails to exit cleanly or a stty command with a stupid set of options will leave the tty in a minimally usable but weird state. I prefer option 1. Primarily because in practice the only things these days that users seem to want to change are the canonical mode special chars. I can't recall the last time I needed to, or heard of anyone else needing to, change attributes like the parity, or enable translating uppercase to lowercase, or having the tty driver convert tabs to spaces. The only exception to this is enabling or disabling software flow control.

The problem with a way to signal to elvish that it should "run this command, and retain the terminal settings it makes" is it will be hard to discover. Yes, to the extent the whitelist and blacklist approaches might surprise a user who deliberately attempts to do stty -icanon, for example, I would argue that user deserves to be surprised 😄 The solution might be to display a warning message that the tty was not in a sane state and elvish corrected the problem.

@xiaq
Copy link
Member

@xiaq xiaq commented Aug 1, 2018

I opened #732.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.