RFR: 8314327: Issues with JShell when using "local" execution engine
Archie Cobbs
acobbs at openjdk.org
Wed Aug 16 15:03:37 UTC 2023
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 ClassLoader.loadClass (ClassLoader.java:588)
| at ClassLoader.loadClass (ClassLoader.java:521)
| at Class.forName0 (Native Method)
| at Class.forName (Class.java:391)
| at Class.forName (Class.java:382)
| at (#1:1)
jshell> Thread.currentThread().getContextClassLoader();
$2 ==> jdk.jshell.execution.DefaultLoaderDelegate$RemoteClassLoader at 45283ce2
jshell> Arrays.asList(((URLClassLoader)$2).getURLs())
$3 ==> []
jshell> new File("classes/test/Foo.class").exists()
$4 ==> true
**Issue 3**
JShell defines a `LoaderDelegate` interface, and provides a perfectly reasonable default implementation in `DefaultLoaderDelegate`, but makes it impossible for anyone wishing to customize `LocalExecutionControl` to leverage any of the existing `DefaultLoaderDelegate` implementation, because the class is not public. Also, this class is non-trivial and so duplicating its tricky class loading function is tedious and error-prone.
There's no reason not to make `DefaultLoaderDelegate` a public class, because (a) it adds no API surface beyond what is already defined by the already-public `LoaderDelegate` interface, and (b) its current behavior is already being relied upon (it's always used), so by making it public no additional dependencies or expectations would be created beyond those that already exist.
-------------
Commit messages:
- Make the DefaultLoaderDelegate class public.
- Apply the "--class-path" flag to the "local" execution engine.
- Add new "contextLoaderParent" parameter for LocalExecutionControlProvider.
- Allow specifying a parent class loader when creating a LocalExecutionControl.
- Allow specifying a parent class loader when creating a DefaultLoaderDelegate.
Changes: https://git.openjdk.org/jdk/pull/15311/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15311&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8314327
Stats: 303 lines in 6 files changed: 296 ins; 1 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/15311.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15311/head:pull/15311
PR: https://git.openjdk.org/jdk/pull/15311
More information about the kulla-dev
mailing list