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

[Archie Cobbs]

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

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

@lahodaj any opinions on the best way to proceed? Thanks for taking the time to review.

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

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