RFR: 8314327: Issues with JShell when using "local" execution engine [v4]

[Jan Lahoda]

On Thu, 5 Oct 2023 23:08:57 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> There are a few bugs (or "opportunities for improvement") in JShell's support for "local" execution control.
>> 
>> **Background**
>> 
>> JShell utilizes two distinct "class lookup" functions. First, it looks up clases when compiling snippets. This happens indirectly via the Java compiler using its `JavaFileSystem` interface. Secondly, JShell executes code by supplying an execution engine with class files and asking it to invoke methods. The execution engine therefore performs a class lookup function when it links and loads these classes. The compilation happens on the local machine while the execution happens either locally or remotely.
>> 
>> For everything to run smoothly, both of these lookup functions must be working for whatever class(es) you want to name in JShell input.
>> 
>> Local execution is implemented by `LocalExecutionControl` and provided by the `LocalExecutionControlProvider` under the name `"local"`. In turn, `LocalExecutionControl` uses a `LoaderDelegate` to manage class loading. The default `DefaultLoaderDelegate` uses a custom subclass of `URLClassLoader` (called `RemoteClassLoader`). This class loader has two functions: (a) keep track of snippet classes generated by JShell, and (b) find classes for the execution engine (normal class loader function).
>> 
>> **Issue 1**
>> 
>> The `RemoteClassLoader` is hard-wired with the system class loader as its parent loader. This means that in non-trivial class loading scenarios, such as when running in web servlet container, the application's classes are not visible to the local execution engine.  As a result, it's impossible to resolve any of these classes.
>> 
>> **Issue 2**
>> 
>> JShell has a `--class-path` parameter that is supposed to handle properly configuring *both* lookup functions, i.e., compilation and execution. Internally, this is done by (a) configuring the compiler's class path, and (b) passing this flag to the execution engine as a "remove VM option".
>> 
>> However, the "local" execution engine ignores the remote VM options.
>> 
>> Here's a simple demonstration:
>> 
>> 
>>     $ ls classes/test/Foo.class
>>     classes/test/Foo.class
>>     $ jshell --execution=local --class-path classes
>>     |  Welcome to JShell -- Version 20.0.1
>>     |  For an introduction type: /help intro
>> 
>>     jshell> Class.forName("test.Foo");
>>     |  Exception java.lang.ClassNotFoundException: test.Foo
>>     |        at URLClassLoader.findClass (URLClassLoader.java:445)
>>     |        at DefaultLoaderDelegate$RemoteClass...
>
> Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add missing @param and @since tags for new constructor.

Looks reasonable to me.

-------------

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15311#pullrequestreview-1672565411