RFR: JDK-8202105: jshell tool: on exiting, terminal echo is disabled

[Jan Lahoda]

On 25.4.2018 22:59, Xueming Shen wrote:
> On 04/25/2018 01:39 PM, Jan Lahoda wrote:
>> So, if I understand correctly, it would be:
>> boolean flipEcho;
>> and the readPassword would do something like:
>> if (echo0() != false) {
>
> if (echo0()) { ...
>
>>     flipEcho = true;
>>     echo(false);
>> }
>> ....
>> if (flipEcho) { //this would also be in the shutdown hook
>>      echo(!echo0());
>> }
>
> if (flipEcho) {
>      echo(true);
>      flipEcho = false;
> }
> ?

Hmm, right. It start to look a lot like the original code, with the 
exception that the flag is also checked inside readPassword:
http://cr.openjdk.java.net/~jlahoda/8202105/webrev.01/

(This also makes the shutdown hook registration lazy.)

Jan

>
>>
>> I guess I can do that (the variant with two boolean feels to me
>> slightly easier to understand and a tiny bit more robust, but this
>> flip boolean variant is doable as well).
>>
>> Thanks,
>>     Jan
>>
>> On 25.4.2018 21:07, Martin Buchholz wrote:
>>> I keep hoping for something simpler.
>>>
>>> Is it possible to have more than one Console object?  Looks like NO.
>>> Assuming no, then you simply need one static flag
>>>
>>> boolean restoreEcho;
>>>
>>> (it could also be an instance field of Console - that would be slightly
>>> more principled)
>>>
>>> In readPassword you check current value of echo and set restoreEcho
>>> if it
>>> was changed.  Shutdown hook also checks the same restoreEcho.
>>>
>>> For bonus points, only create the shutdown hook the first time
>>> readPassword
>>> is called with echo on to appease the Emacs shell users with pitchforks.
>>>
>>> On Wed, Apr 25, 2018 at 9:34 AM, Xueming Shen <xueming.shen at oracle.com>
>>> wrote:
>>>
>>>> On 4/25/18, 9:02 AM, Martin Buchholz wrote:
>>>>
>>>> It would be more correct I think for Console to track if there is a
>>>> pending readPassword in progress and try to restore echo on exit
>>>> only if
>>>> so.  But a little annoying to implement (need an additional boolean?)
>>>>
>>>>
>>>> I think that is what Jan proposed "to add restoredEchoOnShutdown =
>>>> true",
>>>> and I'm fine
>>>> with that. I'm just a little confused why jshell invokes
>>>> System.console()
>>>> if it handles all "raw"
>>>> terminal stuff itself.
>>>>
>>>>
>>>> On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen <xueming.shen at oracle.com>
>>>> wrote:
>>>>
>>>>>
>>>>> Hi Jan,
>>>>>
>>>>> I saw System.console() returns null inside jshell ... but it seems
>>>>> there
>>>>> are 2 vms.
>>>>> I would assume jshell  itself sets the terminal to raw and then call
>>>>> System.console()?
>>>>> for example an alternative for this issue is to ask jshell's impl
>>>>> to call
>>>>> System.console()
>>>>> before going into raw mode? No, I'm not saying the proposed one is
>>>>> not a
>>>>> good one,
>>>>> just wanted to make sure I understand the situation correctly.
>>>>>
>>>>> Thanks,
>>>>> Sherman
>>>>>
>>>>>
>>>>>
>>>>> On 4/25/18, 4:50 AM, Jan Lahoda wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Under:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8194750
>>>>>>
>>>>>> j.i.Console was changed to capture the state of the terminal echo at
>>>>>> creation time, and to restore it on shutdown.
>>>>>>
>>>>>> That is problematic at least in jshell, where the terminal is
>>>>>> already in
>>>>>> the raw mode when j.i.Console is created, and so "echo disabled" is
>>>>>> recorded there. So even though jshell itself sets the terminal
>>>>>> into the
>>>>>> original mode when it terminates, the shutdown hook in
>>>>>> j.i.Console, which
>>>>>> is run later, sets the echo to off even if it was enabled before
>>>>>> the VM
>>>>>> started.
>>>>>>
>>>>>> My understanding is that the shutdown hook is only needed in case
>>>>>> the VM
>>>>>> goes down while readPassword is running. So I tried to change the
>>>>>> shutdown
>>>>>> hook to only work while readPassword is running.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202105
>>>>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8202105/webrev.00/
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Thanks,
>>>>>>      Jan
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>