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

[Jan Lahoda]

On Wed, 4 Oct 2023 22:19:27 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Update tests to compile source instead of including bytecode.
>  - Merge branch 'master' into JShellStuff
>  - 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.

Seems adding `createExecutionControl()` to `LocalExecutionControlProvider` should be enough? Something along these lines possibly?

diff --git a/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java b/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
index cc3879c5c6d..370fbb6bdfd 100644
--- a/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
+++ b/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControlProvider.java
@@ -25,6 +25,7 @@
 
 package jdk.jshell.execution;
 
+import java.util.List;
 import java.util.Map;
 import jdk.jshell.spi.ExecutionControl;
 import jdk.jshell.spi.ExecutionControlProvider;
@@ -78,7 +79,31 @@ public Map<String,String> defaultParameters() {
      */
     @Override
     public ExecutionControl generate(ExecutionEnv env, Map<String, String> parameters) {
-        return new LocalExecutionControl();
+        ExecutionControl result = createExecutionControl(env, parameters);
+
+        // Apply any configured class path
+        List<String> remoteOptions = env.extraRemoteVMOptions();
+        int classPathIndex = remoteOptions.indexOf("--class-path") + 1;
+        if (classPathIndex > 0 && classPathIndex < remoteOptions.size()) {
+            try {
+                result.addToClasspath(remoteOptions.get(classPathIndex));
+            } catch (ExecutionControl.ExecutionControlException e) {
+                throw new RuntimeException("error configuring class path", e);
+            }
+        }
+
+        return result;
     }
 
+    /**
+     * Create the {@link ExecutionControl} using the specified delegate.
+     * Called from {@link #generate(ExecutionEnv, Map) }.
+     *
+     * @param env the {@code ExecutionEnv} for which the ExecutionControl should be created
+     * @param parameters the parameters that were passed to {@code generate}
+     * @return the newly created {@code ExecutionControl}
+     */
+    public ExecutionControl createExecutionControl(ExecutionEnv env, Map<String, String> parameters) {
+        return new LocalExecutionControl(new DefaultLoaderDelegate());
+    }
 }

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

PR Comment: https://git.openjdk.org/jdk/pull/15311#issuecomment-1749491932