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

[Robert Field]

Per Jan's review, I have updated the webrev:

     http://cr.openjdk.java.net/~rfield/8156101v3_lang.webrev/

Specifically:
* SPIConstants has been removed (the values moved internal, to Util, and 
made package private).
* The method name is now passed to ExecutionControl.invoke()  -- and 
correspondingly, is now sent over the wire to the RemoteAgent.
* The expunge() method (and calls to it) have been removed from 
JDIExecutionControl and LocalExecutionControl.
* SPIDebugControl has been removed (and InternalDebugControl un-removed, 
but now having the updated guts of SPIDebugControl).
* To rid ourselves of InternalDebugControl in the future, I have 
created: https://bugs.openjdk.java.net/browse/JDK-8157080.
* I have updated LocalExecutionControl to Grigory's latest version 
(which no longer has commented out code).
* I have fixed the javadoc of ClassTracker.ClassInfo.

Bug:
     https://bugs.openjdk.java.net/browse/JDK-8156101

-Robert

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. 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.
>
> -also not sure about SPIDebugControl - is there a chance we could 
> replace that with a group of Loggers, or something similar?
>
> -in KullaTesting I wonder if we could avoid the code duplication and 
> let the setUp() method delegate to setUp(ExecutionControl)
>
> A few nits:
> -commented-out code in LocalExecutionControl.java
> -javadoc of ClassTracker.ClassInfo:
> Associates a class name class bytes and ReferenceType.
> maybe a missing comma between "class name" and "class bytes"?
>
> Jan
>
> 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
>>>>>
>>>>
>>>
>>