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

Ability to stop reading the input stream #140

Closed
gnodet opened this issue Jun 27, 2017 · 16 comments
Closed

Ability to stop reading the input stream #140

gnodet opened this issue Jun 27, 2017 · 16 comments
Milestone

Comments

@gnodet
Copy link
Member

gnodet commented Jun 27, 2017

Related to #138, it would be interesting to have a way to stop reading the main terminal input stream in advance for signal processing.
While adding a couple of methods pause() / resume() / isPaused() to the Terminal interface is easy, the problem is that the implementation can not easily interrupt its reading. So this would require a smarter control of the underlying input stream.
On Windows, it may be achievable using WaitForSingleObject on the console handle to actually wait until there are some events to read. However, this method is missing in the jansi library.
On Unix, we may 2 possibilities that would require investigation: using a FileChannel on top of the InputStream (when it's based on a real unix pipe) and check if the reading thread can actually be interrupted or when using a Pty, it may be possible to set the VTIME flag to set the timeout (in tenth of seconds) so that the read will unblock automatically.

@gnodet
Copy link
Member Author

gnodet commented Sep 22, 2017

Work in progress branch at https://github.com/gnodet/jline3/tree/issue-140

@cjohnstoniv
Copy link

cjohnstoniv commented Dec 23, 2017

So I've solved this so far on windows (haven't looked at any UNIX impls yet), with a less low level way of solving it than described above, however with 1 small bug. Instead of getting low level and using WaitForSingleObject, I instead introduced a pause lock/condition that blocks the pump() method in AbstractWindowsTerminal when a volatile boolean is set and the condition is released (signaled) when resume() is called. This has been working great with 1 minor issue that the first keypress still gets hijacked by the reader and is replayed to the console when resume() is called. This only happens on the first pause/resume cycle, subsequent pauses don't hijack the first input.

Any ideas of why the first input is still hijacked by the reader (Still need to do some debugging on this)? Maybe this is why you wanted to go lower level with WaitForSingleObject? Can you see any other issues (maybe with the UNIX impls) with following the less low level approach of a condition blocking the thread?

@cjohnstoniv
Copy link

cjohnstoniv commented Dec 23, 2017

The above problem can be fixed by having the pause() method block until the pump() loop method officially gets into a paused stage (i.e isn't waiting to read an event). This raises another issue though, pause() blocks until a console event occurs.

This can be fixed a little more natively with JNA by adding a PeekConsoleInput method to the Kernel32 class and within JnaWinSysTerminal#readConsoleInput() first peeking, returning null if no events, otherwise reading the events as normal. Same goes with JANSI except this method is available in the WindowsSupport class so no changes are required besides to JansiWinSysTerminal#processConsoleInput() to peek, return null if no events, otherwise reads the events.

What would be better waiting n milliseconds and timing out with WaitForSingleObject? Or a continuous peek and poll? There is some pros/cons to both. Waiting n milliseconds means that we'd always have to incur (at most) an n millisecond wait after calling pause() for it to official be paused and this requires changes to JANSI. But we wouldn't incur the constant CPU/overhead/duplication of the continuous peek and poll. On the other hand the peek and poll model helps minimize that (at most) n millisecond wait after calling pause() and comes without requiring any changes to JANSI.

@cjohnstoniv
Copy link

cjohnstoniv commented Dec 25, 2017

PosixSysTerminal on Cygwin works out of the box, no changes necessary. Signals (like CTRL + D) are passed through to the underlying process. PosixPtyTerminal requires a similar pause strategy but picks an InterruptedIOException and handles it.

gnodet added a commit to gnodet/jline3 that referenced this issue Jan 5, 2018
This commit is currently mostly about using non blocking input streams in the Terminal.
gnodet added a commit to gnodet/jline3 that referenced this issue Jan 5, 2018
@gnodet
Copy link
Member Author

gnodet commented Jan 5, 2018

If you want to give a try to the new PR, that would be nice. The basics are here I think, but I haven't done much testing.

gnodet added a commit to gnodet/jline3 that referenced this issue Jan 5, 2018
gnodet added a commit that referenced this issue Jan 8, 2018
This commit is currently mostly about using non blocking input streams in the Terminal.
gnodet added a commit that referenced this issue Jan 8, 2018
gnodet added a commit that referenced this issue Jan 8, 2018
@gnodet
Copy link
Member Author

gnodet commented Jan 8, 2018

I've merged the commits above in master.

@cjohnstoniv
Copy link

What is the anticipated release date of 3.6.0? I should be able to find some time tonight to test master, as for the last several weeks I've been using my own locally edited version of my own implementation. Been a bit busy so apologies for the delay in testing this for you.

@gnodet
Copy link
Member Author

gnodet commented Jan 26, 2018

Nothing's set in stone for the release date, but I'd like to get it out in a 2-3 weeks max, maybe sooner if everyone's happy with it.

@cjohnstoniv
Copy link

Is there a local Jansi change you have? I've cloned Jansi 1.17-SNAPSHOT but am getting "JansiWinSysTerminal.java:[75,46] error: method readConsoleInput in class WindowsSupport cannot be applied to given types;" when compiling JLines master branch.

@gnodet
Copy link
Member Author

gnodet commented Jan 26, 2018

Yes, sorry I forgot to commit the jans-native change, see fusesource/jansi@402e4ae

@cjohnstoniv
Copy link

Confirmed this works from Windows over both CMD Prompt and Cygwin! Please let me know if there is anything else I can do to help.

@cjohnstoniv
Copy link

I have upgraded to 3.6.0 and am seeing some bugs in Windows 10 command prompt. After a pause and resume, my input gets messy. I'll push a key and sometimes it won't be recognized (at least no output on the screen) and others the same key gets output many times. Any ideas? I haven't tested yet in other terminals and I'll do some deeper digging later.

@cjohnstoniv
Copy link

cjohnstoniv commented Feb 8, 2018

This happens when the pause() and resume() are toggled quickly. Shouldn't the pause() method block until the console is truly paused?

In the implementation I had written, I had the pause method block until the console pauses, throwing an interrupted exception. This also would protect the pump reader thread from potentially grabbing quickly entered input after the pause().

@gnodet
Copy link
Member Author

gnodet commented Feb 8, 2018

Yeah, I think there's definitely a possible problem if the the pump thread is not done before resume is called. Would you be able to provide a PR ?

@cjohnstoniv
Copy link

Yes, I can find some time tonight to do this.

@cjohnstoniv
Copy link

cjohnstoniv commented Feb 9, 2018

See the pull request for this: #228

I've test it on Windows command prompt and cygwin.

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