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

vty 5.2.9 seems to not restore input state #77

Closed
jtdaugherty opened this issue Jun 29, 2015 · 8 comments
Closed

vty 5.2.9 seems to not restore input state #77

jtdaugherty opened this issue Jun 29, 2015 · 8 comments

Comments

@jtdaugherty
Copy link
Owner

I'm not sure what could be going on here, but I'm running into a situation where once vty has shut down, I see a strange behavior in the terminal input. Here is a minimal test program that exhibits the behavior for me on 5.2.9:

module Main where

import Data.Default
import Graphics.Vty

main :: IO ()
main = do
    vty <- mkVty def
    shutdown vty

    putStrLn "Enter a line of text:"
    s <- getLine
    print s

The expected behavior: once I see the prompt, I enter some text, press Enter, and see that text printed.

The actual behavior: once I see the prompt, I enter some text, press Enter, nothing happens, I enter more text, press Enter, and the second line of text I entered is printed (so the first is ignored).

@jtdaugherty
Copy link
Owner Author

Upon further investigation, it looks like the problem might be that the input-processing thread that vty forks is getting the first line of input after vty has appeared to shut down. If I enable debug logging, the first line of text that I enter (that I expect to go to getLine instead) appears in the debug log before I see log messages indicating a vty restart. Here is an excerpt in which I enter "abc(RET)def(RET)" at the prompt. This behavior surprises me, since the documentation states that canonical mode is disabled; doesn't that mean line-buffering like this shouldn't happen? I don't know what the Haskell code for disabling canonical mode looks like, or I'd confirm whether it is being disabled. :)

...
map "screen-256color" "\ESC|" KChar '|' [MMeta]
map "screen-256color" "\ESC}" KChar '}' [MMeta]
map "screen-256color" "\ESC~" KChar '~' [MMeta]
input bytes: "abc\n"
valid parse: EvKey (KChar 'a') []
remaining: "bc\n"
valid parse: EvKey (KChar 'b') []
remaining: "c\n"
valid parse: EvKey (KChar 'c') []
remaining: "\n"
valid parse: EvKey KEnter []
remaining: ""
parsed event: EvKey (KChar 'a') []
parsed event: EvKey (KChar 'b') []
parsed event: EvKey (KChar 'c') []
parsed event: EvKey KEnter []
initial (vmin,vtime): (1,100)
map "screen-256color" "\b" KBS []
map "screen-256color" "\ESC[Z" KBackTab []
...

@jtdaugherty
Copy link
Owner Author

Another data point: in Graphics.Vty.Input.Loop, if I modify initInputForFd so that it returns a shutdownInput function that kills the input thread rather than just writing to its stop IORef, then the above test program behaves as expected:

-    _ <- forkOS $ runInputProcessorLoop classifyTable input stopFlag
-    return input
+    loopThreadID <- forkOS $ runInputProcessorLoop classifyTable input stopFlag
+    return input { shutdownInput = killThread loopThreadID }

(Granted, the shutdownInput available inside that thread is the old version that just writes to the IORef, but it never gets called from within there anyway as far as I can tell.)

I'm not entirely confident this is a Good Idea, but at the very least it confirms that the input thread was sticking around.

@jtdaugherty
Copy link
Owner Author

I'd love to get a fix released for this since it's the last major hurdle before I feel comfortable releasing brick. However, while I'm happy to submit a pull request to make the change I described above, I'm curious to hear your thoughts on the proposed solution since it feels slightly hackish to me and I'm wondering if there's a better one.

@coreyoconnor
Copy link
Contributor

A killThread is safe enough in this case AFAICT. Though I'm not sure if this breaks the vmin/vtime handling. That requires the read (fdReadBuf in this case) to be done in blocking mode. Which is something that I frequently break because:

  1. I don't use it
  2. Requires carefully stepping around the IO manager. Which wants to do everything in nonblocking mode on Fds.

So while the above patch fixes this issue I don't know if it causes a regression in vmin/vtime support.

@jtdaugherty
Copy link
Owner Author

What would the consequences of broken vmin/vtime support be?

@jtdaugherty
Copy link
Owner Author

As an aside, this change now makes this demo work:

https://github.com/jtdaugherty/brick/blob/master/programs/SuspendAndResumeDemo.hs

Thanks!

@coreyoconnor
Copy link
Contributor

I've been verifying vmin and vtime support. Looks like this change did not break vmin/vtime. LGTM!

The defaults for vty are a vmin of 1 and a vtime of 1. In Vty's Config structure this would be a vtime of 100. With a vmin/vtime of 1 the read should block indefinitely until input is available. Then return the input immediately (since vmin of 1 will be immediately satisfied). In my testing this always results in the returned bytes containing all the bytes necessary to process an escape sequence for a special key.

In other words, the input bytes for a special key are never split across multiple reads.

In the worse case, if vmin and vtime are broken then classification of input reads for escape sequences would be broken. This would break function keys, arrow keys etc. This is somewhat easy to check: Use EventEcho or Demo and verify special keys work as expected. (Assuming TERM is correct) Though this should be done on a real terminal, not emulator, to make sure characteristics unique to a real serial tty are handled properly. Eh.

The case I'm worried about is the one I don't use: AFAICT emacs users have the expectation to be able to press escape then another key and have that interpreted as meta+key. This is done by setting vmin to > 1 and vtime to > 0. I check this by using Demo.hs and verifying that quickly pressing escape then a letter results in the event: EvKey (KChar _) [MMeta]

Previously this has also been related to performance issues. The vmin/vtime settings have no effect if the file descriptor is in non-blocking mode. At one time the FD was not placed in blocking mode, but vmin/vtime were set to blocking values. The read was returning immediately and vty called read in a loop. High CPU usage just polling the input FD as fast as possible.

In investigating this I noticed the configuration of vmin and vtime is broken: The default of 1 and 100 are always used. Fixed in 954e0f1

This change also removes the need to explicitly use standardIOConfig. That will be added if those properties are missing. This shouldn't change any users of standardIOConfig already.

Originally vty tried to use nonblocking mode and emulated vmin, vtime. This emulation did not work, but I suspect this is still possible.

@jtdaugherty
Copy link
Owner Author

Thanks for the detailed explanation.

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

No branches or pull requests

2 participants