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

2018-04-26 Thread Xueming Shen 

+1

(nit: remove the "where:" and the corresponding indent before push please).


On 4/26/18, 1:34 PM, Jan Lahoda wrote:



Ok, here is an updated webrev:
http://cr.openjdk.java.net/~jlahoda/8202105/webrev.02/

Jan

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

2018-04-26 Thread Jan Lahoda 

On 26.4.2018 20:09, Xueming Shen wrote:

On 4/26/18, 3:23 AM, Jan Lahoda wrote:

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

shouldn't we move the installShutonwHookIfNeeded() into synced block? and
simply name it "installShutdownHook()" ?


Ok, here is an updated webrev:
http://cr.openjdk.java.net/~jlahoda/8202105/webrev.02/

Jan



the rest looks fine.

-sherman

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

2018-04-26 Thread Xueming Shen 

On 4/26/18, 3:23 AM, Jan Lahoda wrote:

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

shouldn't we move the installShutonwHookIfNeeded() into synced block? and
simply name it "installShutdownHook()" ?

the rest looks fine.

-sherman

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

2018-04-26 Thread 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 
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 
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

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

2018-04-25 Thread Xueming Shen 

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;
}
?



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 
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 
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

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

2018-04-25 Thread Martin Buchholz 
On Wed, Apr 25, 2018 at 1:34 PM, Jan Lahoda  wrote:

>
> IIRC the call to System.console() in jshell/jline that is part of this
> problem is basically a way to call Console.istty() - the returned Console
> is not used. I was considering tweaking jshell to avoid this issue, and I
> think it would be possible, but seemed some other programs might run into
> issues as well, so seemed better to fix in Console.


So the shutdown hook is unneeded both by Emacs users and by JShell users,
strengthening the case for optimizing it away.

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

2018-04-25 Thread Jan Lahoda 

So, if I understand correctly, it would be:
boolean flipEcho;
and the readPassword would do something like:
if (echo0() != false) {
flipEcho = true;
echo(false);
}

if (flipEcho) { //this would also be in the shutdown hook
 echo(!echo0());
}

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 
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 
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

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

2018-04-25 Thread Jan Lahoda 

Hi Sherman,

JShell uses two processes - the main one interacts with the terminal and 
compiles the user's snippets. The other one runs the snippets, and is 
not connected to a terminal, so System.console() does not work there.


IIRC the call to System.console() in jshell/jline that is part of this 
problem is basically a way to call Console.istty() - the returned 
Console is not used. I was considering tweaking jshell to avoid this 
issue, and I think it would be possible, but seemed some other programs 
might run into issues as well, so seemed better to fix in Console.


Jan

On 25.4.2018 17:52, Xueming Shen 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

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

2018-04-25 Thread John Rose 
On Apr 25, 2018, at 12:07 PM, Martin Buchholz  wrote:
> 
> 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.

FTR, I use Emacs shell for everything except jshell, and have to
launch a separate shell app. (Terminal) to run jshell.  That's the only
reason I launch that app.  My pitchfork is still in the shed, but I feel
the pain here too.  There's probably a config. switch I'm missing,
but jshell has never worked for me under Emacs.

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

2018-04-25 Thread Martin Buchholz 
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 
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 
> 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
>>>
>>
>>
>
>

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

2018-04-25 Thread Xueming Shen 

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 > 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

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

2018-04-25 Thread Martin Buchholz 
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?)

On Wed, Apr 25, 2018 at 8:52 AM, Xueming Shen 
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
>>
>
>

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

2018-04-25 Thread Xueming Shen 


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