RFR JDK-8167461: jshell tool: Scanner#next() hangs tool

[Jan Lahoda]

Hi,

Updated webrev is here:
http://cr.openjdk.java.net/~jlahoda/8167461/webrev.01/

In the end, I used InterruptedIOException instead of some newly created 
StopException, as it seems reasonably close to what we need.

The userin data that are being sent from the engine to the remote agent 
are tagged now, and allow to send data, close the stream, and partially 
transfer exceptions (just the exception's message is transferred, not 
the stack trace).

What do you think?

Thanks,
     Jan

On 13.10.2016 21:29, Robert Field wrote:
> Looking forward to it!
>
> -Robert
>
> On 10/13/16 11:53, Jan Lahoda wrote:
>> On 12.10.2016 22:52, Robert Field wrote:
>>> Looks good.  Might be safest to copy the checks from the top of
>>> InputStream's implementation --
>>>
>>>      public int read(byte b[], int off, int len) throws IOException {
>>>          if (b == null) {
>>>              throw new NullPointerException();
>>>          } else if (off < 0 || len < 0 || len > b.length - off) {
>>>              throw new IndexOutOfBoundsException();
>>>          } else if (len == 0) {
>>>              return 0;
>>>          }
>>
>> Yes, makes sense.
>>
>>>
>>> I'm somewhat unclear on why the read/write in Util are changed to the
>>> buffered form.  BTW.  There isn't (and wasn't) an EOF test here.
>>
>> The reason is that (currently), when the user's snippet is waiting for
>> input and the user types Ctrl-C, the user's snippet is stopped, but
>> the read method returns -1 (see ConsoleIOContext.readUserInput())
>> (somewhat pretending stream end). So this change was to prevent
>> writing this -1 to the remote side.
>>
>> But, I was thinking how to improve that (and add/fix handling of EOF
>> as well, even though I guess it might be surprising if the user in is
>> closed). And we could do this:
>> -add a new exception, like StopException (similar to JLine's
>> UserInterruptException)
>> -the (user-)in.read() could throw the exception to signal the user
>> cancelled the input. execution.Util.remoteInputOutput would then take
>> care of cancelling the user's code, and would know to not pass any
>> data to the agent.
>>
>> What do you think?
>>
>> Thanks,
>>    Jan
>>
>>>
>>> As to input carrying over: I can see either as being surprising in some
>>> cases.  As-is, very simple "Enter two numbers: " kinds of newbie coding
>>> will misbehave, and misbehave in a way that could look like a hang
>>> (waiting on input).
>>> With a input carry-over implementation however, garbage entered, perhaps
>>> much earlier in the session would still be what is read later -- that
>>> could be hard to diagnose, and would take coding (or a /reset) to cure.
>>> I think addressing that should be a separate issue.
>>>
>>> -Robert
>>>
>>> On 10/12/16 12:41, Jan Lahoda wrote:
>>>> Hello,
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8167461
>>>>
>>>> The problem is that this sequence of snippets is not working properly
>>>> (won't read the System.in properly/seemingly hangs):
>>>> jshell> Scanner input = new Scanner(System.in);
>>>> jshell> int x = input.nextInt();
>>>>
>>>> The reason is that the PipeInputStream (which is used to pass data
>>>> received by the remote agent from the socket to System.in) does not
>>>> override the read(byte[], int, int) method. So the default
>>>> implementation will be used, which waits until all the requested data
>>>> are read. Scanner wrap s the System.in with InputStreamReader, and
>>>> that appears to read (up to) 8192 bytes into a buffer, but the user
>>>> typically does not type so many characters. The solution is to
>>>> override the read(byte[], int, int) method, and return only bytes we
>>>> have buffered (except the method will wait for the first byte).
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~jlahoda/8167461/webrev.00/
>>>>
>>>> Also, more general question related to the System.in handling: unused
>>>> input for one snippet is not passed to following snippets - is that
>>>> OK, or do we need to discuss changing that? I.e.:
>>>> jshell> int x = input.nextInt();
>>>> 1 1
>>>> x ==> 1
>>>> //the other "1" is thrown away
>>>>
>>>> jshell> int x = input.nextInt();
>>>> 2
>>>> x ==> 2
>>>> //the user had to type additional input.
>>>>
>>>> Thanks,
>>>>     Jan
>>>
>