RFR 8169519: JShell: Handle start-up failures and hangs gracefully
Jan Lahoda
jan.lahoda at oracle.com
Tue Nov 22 21:22:18 UTC 2016
Seems OK to me.
Jan
On 22.11.2016 01:41, Robert Field wrote:
> Per off-line review --
>
> Generally looks okay, but ... I don't think the fields
>
> DEFAULT_TIMEOUT
> REMOTE_AGENT
>
> should be constants from a JLS perspective.
> These values are not so constant we might not want to change
> them in the future, but as it is they are constants
> from a Java perspective and thus the raw values will get
> constant-folded and compiled into clients. I recommend
> replacing these constants with static methods instead.
>
> Thanks,
> -Joe
>
> I rev'ed the code again --
>
> Bugs:
>
> 8169519: JShell: Handle start-up failures and hangs gracefully
> https://bugs.openjdk.java.net/browse/JDK-8169519
>
> 8166581: JShell: locks forever if -R options is wrong
> https://bugs.openjdk.java.net/browse/JDK-8166581
>
> Webrev:
>
> http://cr.openjdk.java.net/~rfield/8169519v3.webrev/
>
> Specdiff:
>
> http://cr.openjdk.java.net/~rfield/8169519v3.specdiff/overview-summary.html
>
> JShell API:
>
> http://cr.openjdk.java.net/~rfield/8169519v3.jshellAPI/
>
> The only change in this version is to the JdiDefaultExecutionControl class:
>
> http://cr.openjdk.java.net/~rfield/8169519v3.specdiff/jdk/jshell/execution/JdiDefaultExecutionControl.html
>
>
> http://cr.openjdk.java.net/~rfield/8169519v3.jshellAPI/jdk/jshell/execution/JdiDefaultExecutionControl.html
>
>
> http://cr.openjdk.java.net/~rfield/8169519v3.webrev/src/jdk.jshell/share/classes/jdk/jshell/execution/JdiDefaultExecutionControl.java.sdiff.html
>
>
> as above, changing the two constants to methods.
>
> Robert
>
>
>
>
> On 11/15/16 19:53, Robert Field wrote:
>> OK, here is v2 -- addresses the first issue, dials-back the second issue.
>> But, much more importantly, after discussions of JDI and some issues
>> there with Alan, I've moved the JShell code to take more control and
>> do more complete failure handling.
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~rfield/8169519v2.webrev/
>>
>>
>> Annotated changes from v1 below --
>>
>> @@@ New tests
>>
>> Only in ./test/jdk/jshell: FailOverExecutionControlDyingLaunchTest.java
>> Only in ./test/jdk/jshell: FailOverExecutionControlHangingLaunchTest.java
>> Only in ./test/jdk/jshell: FailOverExecutionControlHangingListenTest.java
>>
>>
>> @@@ First choice in fail-over sequence is JShell controlled launch:
>>
>> --- ./src/jdk.jshell/share/classes/jdk/jshell/JShell.java Tue Nov 15
>> 13:32:14 2016
>> ***************
>> *** 118,125 ****
>> this.extraCompilerOptions = b.extraCompilerOptions;
>> ExecutionControl.Generator executionControlGenerator =
>> b.executionControlGenerator==null
>> ? failOverExecutionControlGenerator(
>> - JdiDefaultExecutionControl.launch(),
>> JdiDefaultExecutionControl.listen(InetAddress.getLoopbackAddress().getHostAddress()),
>>
>> JdiDefaultExecutionControl.listen(null)
>> )
>> : b.executionControlGenerator;
>> --- 118,125 ----
>> this.extraCompilerOptions = b.extraCompilerOptions;
>> ExecutionControl.Generator executionControlGenerator =
>> b.executionControlGenerator==null
>> ? failOverExecutionControlGenerator(
>> JdiDefaultExecutionControl.listen(InetAddress.getLoopbackAddress().getHostAddress()),
>>
>> + JdiDefaultExecutionControl.launch(),
>> JdiDefaultExecutionControl.listen(null)
>> )
>> : b.executionControlGenerator;
>>
>>
>> @@@ Monitor process termination (listen case) to more quickly
>> fail-over. New parameter to timedVirtualMachineCreation
>>
>> @@@ Put stopListening in a finally clause so connections don't leak
>>
>> diff -r
>> /w/y/dev/langtools/src/jdk.jshell/share/classes/jdk/jshell/execution/JdiInitiator.java
>> ./src/jdk.jshell/share/classes/jdk/jshell/execution/JdiInitiator.java
>> 27a28
>>> import java.io.IOException;
>> 43a45
>>> import com.sun.jdi.connect.IllegalConnectorArgumentsException;
>> 126c128
>> < VirtualMachine new_vm = timedVirtualMachineCreation(()
>> -> launcher.launch(connectorArgs));
>> ---
>>> VirtualMachine new_vm = timedVirtualMachineCreation(() ->
>>> launcher.launch(connectorArgs), null);
>> 160,161c162,163
>> < vm = timedVirtualMachineCreation(() ->
>> listener.accept(connectorArgs));
>> < listener.stopListening(connectorArgs);
>> ---
>>> vm = timedVirtualMachineCreation(() ->
>>> listener.accept(connectorArgs),
>>> () -> process.waitFor());
>> 167a170,175
>>> } finally {
>>> try {
>>> listener.stopListening(connectorArgs);
>>> } catch (IOException | IllegalConnectorArgumentsException
>>> ex) {
>>> // ignore
>>> }
>> 171c179,180
>> < VirtualMachine
>> timedVirtualMachineCreation(Callable<VirtualMachine> creator) throws
>> Exception {
>> ---
>>> VirtualMachine
>>> timedVirtualMachineCreation(Callable<VirtualMachine> creator,
>>> Callable<Integer> processComplete) throws Exception {
>> 173c182
>> < ExecutorService executor =
>> Executors.newSingleThreadExecutor(runnable -> {
>> ---
>>> ExecutorService executor =
>>> Executors.newCachedThreadPool(runnable -> {
>> 178,179d186
>> < Future<VirtualMachine> future = executor.submit(creator);
>> <
>> 181,184c188,202
>> < result = future.get(connectTimeout, TimeUnit.MILLISECONDS);
>> < } catch (TimeoutException ex) {
>> < future.cancel(true);
>> < throw ex;
>> ---
>>> Future<VirtualMachine> future = executor.submit(creator);
>>> if (processComplete != null) {
>>> executor.submit(() -> {
>>> Integer i = processComplete.call();
>>> future.cancel(true);
>>> return i;
>>> });
>>> }
>>>
>>> try {
>>> result = future.get(connectTimeout,
>>> TimeUnit.MILLISECONDS);
>>> } catch (TimeoutException ex) {
>>> future.cancel(true);
>>> throw ex;
>>> }
>>
>>
>> @@@ Add license header
>>
>> diff -r /w/y/dev/langtools/test/jdk/jshell/DyingRemoteAgent.java
>> ./test/jdk/jshell/DyingRemoteAgent.java
>> 0a1,22
>>> /*
>>> * Copyright (c) 2016, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or modify it
>>> * under the terms of the GNU General Public License version 2 only, as
>>> * published by the Free Software Foundation.
>>> *
>>> * This code is distributed in the hope that it will be useful, but
>>> WITHOUT
>>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>>> * version 2 for more details (a copy is included in the LICENSE file
>>> that
>>> * accompanied this code).
>>> *
>>> * You should have received a copy of the GNU General Public License
>>> version
>>> * 2 along with this work; if not, write to the Free Software
>>> Foundation,
>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>> *
>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA
>>> 94065 USA
>>> * or visit www.oracle.com if you need additional information or have
>>> any
>>> * questions.
>>> */
>>
>> @@@ Trim some over-jealous exception message matches
>>
>> diff -r
>> /w/y/dev/langtools/test/jdk/jshell/JdiBadOptionListenExecutionControlTest.java
>> ./test/jdk/jshell/JdiBadOptionListenExecutionControlTest.java
>> 42c42
>> < "Launching JShell execution engine threw: Failed remote
>> listen: java.util.concurrent.ExecutionException:
>> com.sun.jdi.connect.TransportTimeoutException: timeout waiting for
>> connection @ com.sun.jdi.SocketListen";
>> ---
>>> "Launching JShell execution engine threw: Failed remote
>>> listen:";
>> diff -r /w/y/dev/langtools/test/jdk/jshell/StartOptionTest.java
>> ./test/jdk/jshell/StartOptionTest.java
>> 144c144
>> < assertTrue(s.startsWith("Launching JShell execution
>> engine threw: Failed remote launch:"), s);
>> ---
>>> assertTrue(s.startsWith("Launching JShell execution engine threw:
>>> Failed remote"), s);
>>
>>
>> Thanks,
>> Robert
>>
>>
>>
>> On 11/14/16 14:01, Jan Lahoda wrote:
>>> Overall, seems OK to me. Two comments:
>>> -DyingRemoteAgent should have a license header:
>>> http://cr.openjdk.java.net/~rfield/8169519v1.webrev/test/jdk/jshell/DyingRemoteAgent.java.html
>>>
>>> -the tests seem to depend on exact exception messages - is there a
>>> risk of that being unstable? (And, possibly, platform-specific?)
>>>
>>> Jan
>>>
>>> On 14.11.2016 20:14, Robert Field wrote:
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~rfield/8169519v1.webrev/
>>>>
>>>> Change of how localhost determined (thanks, Jan). Tests not
>>>> included in
>>>> webrev,
>>>>
>>>> -Robert
>>>>
>>>> On 11/09/16 20:01, Robert Field wrote:
>>>>> JShell: Handle start-up failures and hangs gracefully --
>>>>> specifically --
>>>>> Add time-outs so hangs do not lock out users and so fail-over can
>>>>> function.
>>>>> Handle failures more gracefully, including cleaning up remote process.
>>>>> Fail-fast for better user-experience and better failure handling.
>>>>> Surface failure in API.
>>>>> Add testing hooks.
>>>>> Better failure information.
>>>>>
>>>>> Bugs:
>>>>>
>>>>> 8169519: JShell: Handle start-up failures and hangs gracefully
>>>>> https://bugs.openjdk.java.net/browse/JDK-8169519
>>>>>
>>>>> 8166581: JShell: locks forever if -R options is wrong
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166581
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~rfield/8169519v0.webrev/
>>>>>
>>>>> Specdiff:
>>>>>
>>>>> http://cr.openjdk.java.net/~rfield/8169519v0.specdiff/overview-summary.html
>>>>>
>>>>>
>>>>>
>>>>> JShell API:
>>>>>
>>>>> http://cr.openjdk.java.net/~rfield/8169519v0.jshellAPI/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Robert
>>>>>
>>>>
>>
>
More information about the kulla-dev
mailing list