RFR 8169519: JShell: Handle start-up failures and hangs gracefully
Robert Field
robert.field at oracle.com
Wed Nov 16 03:53:26 UTC 2016
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