REPL tool implementation code review

[Paul Sandoz]

On 7 Sep 2015, at 11:52, Jan Lahoda <jan.lahoda at oracle.com> wrote:
>>> jdk/internal/jshell/tool/JShellTool.java:307
>>>> byte[] encoded = Files.readAllBytes(Paths.get(filename));
>>>> cmdlineStartup = new String(encoded);
>>> 
>>> 
>>> What character set is the file encoded in? should the platform's default charset be used or UTF-8? Same applies to processing the load list files.
> 
> I'll leave the final answer on Robert, but I'd expect the platform's default charset to be used - the file is provided by the user on the command line, so I'd assume it is in the platform's encoding.
> 

I would assume so too, but just wanted to double check.


>>> 
>>> 
>>>> if (path.isEmpty()) {
>>>>     for (Path root : FileSystems.getDefault().getRootDirectories()) {
>>>>         if (!accept.test(root) || !root.toString().startsWith(prefix))
>>>>             continue;
>>>>         result.add(new Suggestion(root.toString(), false));
>>>>     }
>>>> }
>>> 
>>> Alas we don’t have a FileSystem.rootDirectories method that returns a stream.
>> 
>> The array is two small to worth a dedicated stream method, no ?
>> and one can use, Arrays.stream(...).
> 
> I'll use StreamSupport.stream to get the Stream.
> 

Ah, i recall now it returns an Iterable. I dunno if it is worth it, at least transform to if (X && Y) and remove the “continue.


>> 
>>> 
>>> 
>>> 
>>> jdk/internal/jshell/tool/JShellTool.java:645
>>>> registerCommand(new Command("/list", "/l", "[all]", "list the source you have typed",
>>>>                             arg -> cmdList(arg),
>>>>                             new FixedCompletionProvider("all")));
>>> 
>>> 
>>> You could use a method reference for "arg -> …”, same applies for other command declarations.
> 
> Not all of the command actions can be converted to method references currently (some of the methods don't need "arg"), so I'd slightly prefer to keep the code consistent and keep the lambdas there. But, if method refs would be strongly preferred, I can put them there.
> 

Up to you, consistency is a good reason to leave as is.


> 
>>> 
>>>> if (!key.pollEvents().isEmpty()) {
>>>>     if (!input.terminalEditorRunning()) {
>>>>         saveFile();
>>>>     }
>>>> }
>>> 
>>> Why does the watcher thread need to call saveFile when it is also performed after the process running the external editor has terminated? I am guessing every time the file is saved in the editor it is processed by the REPL?
> 
> Yes, I think the intent is that when the (GUI) editor saves the file, it is immediately processed by REPL, so the user can see the outcomes without quiting the editor.
> 
> Note this processing is disabled when the editor is detected to be a terminal editor (as processing the input while the terminal editor is running would make a mess of the screen).
> 

Ok.


>>> 
>>> 
>>> jdk/internal/jshell/tool/ConsoleIOContext.java:
>>> 
>>> I am struggling to understand the interactions of the before/afterUse code blocks, the CircularBuffer, and the thread created for writing (or pushing) read values to the buffer.
>>> 
>>> CircularBuffer feels like a blocking queue, but there is some additional communication signalling how much input is needed.
> 

It would be useful to clear up all the access levels of ConsoleIOContext and the inner class as that would help in terms of knowing who can do what e.g. make some/all inner classes of ConsoleIOContext private and then make ConsoleIOContext package private, since it’s constructor is. Perhaps some unit tests require certain access, mainly related to EditingHistory? if so could that be made a top-level class with a comment as to why it is public?

You might need to make InputHandler.reading volatile as it is accessed by two different threads (the thread reading InputHandler.reading may never observer the write in another thread).


> This is a tricky code trying to resolve interaction between two of our "special" features:

Indeed.

Have i got the following correct?

1) reading a line, processing user code, and terminal editing are three mutually exclusive states

2) the underlying input.read may block since a request to read is not an indication of there being anything available to be read

3) when processing user code the waiting until requested input is effectively disabled, thus any characters are buffered asynchronously ready for processing when the next line is requested.

4) when processing user code if a CTRL-C is observed then the REPL is asynchronously stopped.

I wonder if it is possible to replace InputHandler.reading and CircularBuffer.inputNeeded with an enum representing those three states.

enum {
  // The state of waiting for a request to read from the underlying input
  WAIT,
  // The state of reading from the underlying input stream and propagating
  READ_AND_PROPAGATE,
  // The state of reading from the underlying input stream and buffering
  READ_AND_BUFFER;
}

That assumes you can do something like the following in InputHandle:

public String readLine(String prompt) throws IOException {
    inputBuffer.s = BufferState.READ_AND_PROPAGATE;
    try {
        return consoleReader.readLine(prompt);
    } finally {
        inputBuffer.s = BufferState.WAIT;
    }
}

public void beforeUserCode() {
    inputBuffer.s = BufferState.BUFFER;
}

public void afterUserCode() {
    inputBuffer.s = BufferState.WAIT;
}

And on CircularBuffer:

public synchronized void waitInputNeeded() {
    while (s == BufferState.WAIT) {
        try {
            wait();
        } catch (InterruptedException ex) {
            //ignore
        }
    }
}

Then you can map those states via some comments to the REPL operations. I hacked together something real quick along those lines and it seemed to work.

Is the asynchronous operation on the REPL in 4 an issue?

My inclination is you should try and merge the thread logic in InputBuffer.wrapInIfNeeded into CircularBuffer, then modify and rename CircularBuffer to extend from InputStream, thereby containing all the relevant multi-threaded logic and it becomes an obvious thing being deferred to. You could also add another state CLOSED, which i think more clearly indicates closure rather than continually writing -1 every time you read -1.

Sorry for labouring the point here, but i think this kind of thing is worth spending some time on. Consider someone trying to modify the code in a few years time. Review-wise we could spin this off as a separate bug so as not to block the code going in.

Paul.


> a) ability to stop/kill user's code by pressing Ctrl-C
> b) run terminal editor in /edit (as opposed to GUI editors, which don't need to use the terminal)
> 
> For "stop", we need to read from the input to detect the Ctrl-C - in particular we need to read from the input while the user's code is running to detect the stop request.
> 
> For terminal editors, we need to let the terminal editor work with the terminal as it wants - in particular, we must not read from the input (otherwise, not all of the user's input would reach the terminal editor).
> 
> A complicating aspect is that (to my knowledge), we can't do a non-blocking read from System.in.
> 
> So the solution (not saying there can't be a better one) is to only read from the input if we know we will process the input character:
> -during ConsoleReader.readLine only if/when the ConsoleReader actually requests to read from the input (because it will then process the character) - see the InputStream returned from ConsoleIOContext.InputHandler.wrapInIfNeeded.
> 
> -when the user's code is running (i.e. inside the beforeUserCode-afterUserCode pair) - but, in this case, we only need to process the Ctrl-C character (i.e. (int) 3) - we need to do something with the rest, and the solution is to stash it into a buffer, so that it is not lost and can be processed in the future.
> 
> This way we are not doing System.in.read() (or alike) while the terminal editor is running, so that it gets the input characters that the user types, but still can read as much as we need while the user's code is running.
>