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

[Archie Cobbs]

On Wed, 4 Oct 2023 18:32:11 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

> 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)

I agree it makes sense to keep the API changes minimal. The problem is that the `--class-path` fix entails adding the path components to whatever `LoaderDelegate` you are using via `addToClasspath()`, so the two problems we're trying to solve intersect.

In other words, consider your first option:
* It requires you to write your own `ExecutionControlProvider`
* The fix for the `--class-path` bug is part of `LocalContextExecutionControlProvider`
* There currently no way to subclass `LocalContextExecutionControlProvider` and also inherit that fix

So you're stuck with choosing between being able to supply a parent `ClassLoader` or having the `--class-path` bug fixed, but not both (or duplicating the bug fix in your own implementation).

Here's another way it could work:

* Start with your first option, i.e., add the new `LocalExecutionControl` constructor
* Making these two new methods in the patch for `LocalExecutionControlProvider` protected and therefore overriddable by subclasses:
  * `createLoaderDelegate()`
  * `createExecutionControl()`

Of course that also adds API surface.

An alternative could be encapsulating the bug fix in its own static method in `LocalExecutionControlProvider`:

public static void applyRemoteVMOptions(ExecutionEnv env, LoaderDelegate delegate) { ... }

or even as a default method in `LoaderDelegate`:

default void initialize(ExecutionEnv env, Map<String, String> parameters) { ... }

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

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