RFR 8169519: JShell: Handle start-up failures and hangs gracefully
Jan Lahoda
jan.lahoda at oracle.com
Wed Nov 16 15:59:22 UTC 2016
Seems OK.
Jan
On 16.11.2016 04: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