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

[Jan Lahoda]

On Wed, 16 Aug 2023 14:55:54 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$RemoteClassLoader.findClass (DefaultLoaderDelegate.java:154)
>     |        at ...

I apologize to belated answer.

So, it seems to me that there are only two problems:
1. `--class-path` is not reflected. That clearly should be fixed
2. the underlying `ClassLoader` cannot be set easily

For 2., there appear to be three new pieces of API to do that: 
- making `DefaultLoaderDelegate` public, and adding a constructor accepting the base `ClassLoader` to it.
- adding a new constructor to `LocalExecutionControl`, accepting the base `ClassLoader`
- adding ability to use `Thread`'s context `ClassLoader`

Having three different APIs that only avoid implementing custom `LoaderDelegate` seems a bit excessive to me, unless there's a strong reason for them. I would suggest to settle on one way, and delete the others. Offhand, my preference order would be:
- new `LocalExecutionControl` constructor (it is the smallest API surface, so as long as it solves the problem, it seems like the obvious solution)
- making `DefaultLoaderDelegate` public (the behavioral compatibility we might need to keep could be non-trivial)
- using context CL (I don't quite see the benefit of this, if we can add satisfactory real API, and the API surface seems pretty big, so we are not really saving anything)

Also, each new API element needs to be marked with `@since 22`, and covered by the corresponding CSR.

test/langtools/jdk/jshell/LocalExecutionTestSupport.java line 38:

> 36: 
> 37:     /*
> 38:      * This is a classfile corresponding to this source:

Compiling classes in the test is actually usually easier than inlining the bytecode in the test. One can use the `@compile` tag (which may not be ideal here), or `java.util.spi.ToolProvider` with tool `javac` (that can only compile sources on disk to classfiles on disk, but that would still be OK here), or `javax.tools.ToolProvider` (which allows to compile in memory, full or partial), or the `test/langtools/tools/lib/toolbox/ToolBox.java`. Given the context here, `java.util.spi.ToolProvider` might give good results without embedding bytecode in the test.

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

PR Review: https://git.openjdk.org/jdk/pull/15311#pullrequestreview-1658225733
PR Review Comment: https://git.openjdk.org/jdk/pull/15311#discussion_r1346280835