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

[Robert Field]

On 10/16/16 11:17, Jan Lahoda wrote:
> On 14.10.2016 17:53, Robert Field wrote:
>> Two things --
>>
>> (1) This behavior should be documented.
>
> Sure, updated webrev:
> http://cr.openjdk.java.net/~jlahoda/8167461/webrev.02/

Thumbs up.

>
>>
>> (2) We have "stop()" in the API, but with this, our reference
>> implementation does not use it.  Should it exist in the API?
>> "stop()" also exists in the SPI -- forwarded API -> EC -> remote EC.
>> This pathway would no longer be used by our reference implementation.
>> What does this mean about stop() in the SPI design?
>
> We still call the API version from the StopDetectingInputStream (via 
> the "stop" Runnable filled in the constructor of ConsoleIOContext).

OK

>
> Even if we didn't call the API, then I would still think the stop 
> method should be in the API, for cases where the client would like to 
> implement e.g. a watchdog.
>
> Introducing an alternative way to do the stop while waiting for input 
> is not perfect, but so far, I don't see a better solution. Any ideas?
>
> Thanks,
>     Jan

Thanks,
Robert

>
>>
>> -Robert
>>
>> On 10/14/16 06:16, Jan Lahoda wrote:
>>> 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
>>>>>>
>>>>
>>