RFR 8156101: JShell SPI: Provide a pluggable execution control SPI

[Robert Field]

On 05/16/2016 12:14 AM, Jan Lahoda wrote:
> Overall, seems reasonable to me. Would be good to get a confirmation 
> from the clients, that it matches their needs. 

Grigory was able to port to the SPI in a few hours.

> For the top-level changes, a review from build-dev would be good.
>
> Comments:
> -I am not sure of SPIConstants - is there a chance we could avoid this 
> class? I see that:
> --DOIT_METHOD_NAME is used in invoke - maybe invoke could take the 
> method name as a parameter?
> --PREFIX_PATTERN is used in implementation of 
> ExecutionControl.varValue; but do we want everybody to re-implement 
> the Object->String conversion? I was thinking if a method like:
> ExecutionControl:
> public static String toString(Object value) {...}
> that every ExecutionControl implementation would call wouldn't work 
> better.

The functionality that uses PREFIX_PATTERN is the expunge() method. 
However, expunge() is also called on the result value on the JShell-core 
side, thus its use in an execution engine is redundant and I will remove.

Having the method name be a parameter makes it more general (I was 
trying to keep it tightly tailored) but it gets rid of the last shared 
constant and allows for some possibly interesting extensibility.

So, I have remove SPIConstants.

>
> -also not sure about SPIDebugControl - is there a chance we could 
> replace that with a group of Loggers, or something similar?

Suggestions...

>
> -in KullaTesting I wonder if we could avoid the code duplication and 
> let the setUp() method delegate to setUp(ExecutionControl)

We could, but then we wouldn't be testing the default case (no 
ExecutionControl explicitly specified).

>
> A few nits:
> -commented-out code in LocalExecutionControl.java

I've updated to Grigory's latest code, and this is gone.
> -javadoc of ClassTracker.ClassInfo:
> Associates a class name class bytes and ReferenceType.
> maybe a missing comma between "class name" and "class bytes"?

Fixed.
>
> Jan

Thanks Jan!

-Robert

>
>
> On 14.5.2016 05:55, Robert Field wrote:
>> Updated langtools webrev with additional code comments only (to ease
>> review).  No changes to code:
>>
>>      http://cr.openjdk.java.net/~rfield/8156101v2_lang.webrev/
>>
>> -Robert
>>
>>
>>
>> On 05/13/2016 12:29 AM, Robert Field wrote:
>>> Updated langtools webrev:
>>>     http://cr.openjdk.java.net/~rfield/8156101v1_lang.webrev/
>>>
>>> -Robert
>>>
>>>
>>> On 05/12/2016 11:41 PM, Robert Field wrote:
>>>> Attached is a test for this functionality utilizing a local
>>>> implementation of ExecutionControl courtesy of Grigory Ptashko.
>>>> Thank you Grigory!
>>>>
>>>> All files in test/jdk/jshell.
>>>>
>>>> This test needs one method added to KullaTesting --
>>>>
>>>> diff -r c51b40933e0c test/jdk/jshell/KullaTesting.java
>>>> --- a/test/jdk/jshell/KullaTesting.java    Wed May 11 20:28:22 2016
>>>> +0000
>>>> +++ b/test/jdk/jshell/KullaTesting.java    Thu May 12 23:31:14 2016
>>>> -0700
>>>> @@ -72,6 +72,7 @@
>>>>  import static jdk.jshell.Snippet.Status.*;
>>>>  import static org.testng.Assert.*;
>>>>  import static jdk.jshell.Snippet.SubKind.METHOD_SUBKIND;
>>>> +import jdk.jshell.spi.ExecutionControl;
>>>>
>>>>  public class KullaTesting {
>>>>
>>>> @@ -166,6 +167,21 @@
>>>>          classpath = new ArrayList<>();
>>>>      }
>>>>
>>>> +    public void setUp(ExecutionControl ec) {
>>>> +        inStream = new TestingInputStream();
>>>> +        outStream = new ByteArrayOutputStream();
>>>> +        errStream = new ByteArrayOutputStream();
>>>> +        state = JShell.builder()
>>>> +                .executionEngine(ec)
>>>> +                .in(inStream)
>>>> +                .out(new PrintStream(outStream))
>>>> +                .err(new PrintStream(errStream))
>>>> +                .build();
>>>> +        allSnippets = new LinkedHashSet<>();
>>>> +        idToSnippet = new LinkedHashMap<>();
>>>> +        classpath = new ArrayList<>();
>>>> +    }
>>>> +
>>>>      @AfterMethod
>>>>      public void tearDown() {
>>>>          if (state != null) state.close();
>>>>
>>>>
>>>> On 05/11/2016 10:53 PM, Robert Field wrote:
>>>>> Please review the new Service Provider Interface for using alternate
>>>>> execution implements.
>>>>>
>>>>> Bug:
>>>>>     https://bugs.openjdk.java.net/browse/JDK-8156101
>>>>>
>>>>> Webrevs:
>>>>> http://cr.openjdk.java.net/~rfield/8156101v0_lang.webrev/
>>>>> http://cr.openjdk.java.net/~rfield/8156101v0_ws.webrev/
>>>>>
>>>>> Javadoc:
>>>>>     http://cr.openjdk.java.net/~rfield/jshell_spi/
>>>>>
>>>>> Note: The SPI is tested since it is used by the default JDI
>>>>> implementation (which has no special access).  However, tests using
>>>>> an alternative execution engine will be included.
>>>>>
>>>>> Thanks,
>>>>> Robert
>>>>>
>>>>
>>>
>>