Hi Igor,
I think it might be best to the interrupt() call out. I wanted to see what would happen if we ever got an InterruptedException, so I added the following to the start of Platform.shouldSAAttach(): try { throw new InterruptedException(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); throw new RuntimeException(e); } At the start of the test run, before any tests are actually run, I see the following: failed to get value for vm.hasSAandCanAttach java.lang.RuntimeException: java.lang.InterruptedException at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300) at requires.VMProps.vmHasSAandCanAttach(VMProps.java:327) at requires.VMProps$SafeMap.put(VMProps.java:69) at requires.VMProps.call(VMProps.java:101) at requires.VMProps.call(VMProps.java:57) at com.sun.javatest.regtest.agent.GetJDKProperties.run(GetJDKProperties.java:80) at com.sun.javatest.regtest.agent.GetJDKProperties.main(GetJDKProperties.java:54) Caused by: java.lang.InterruptedException at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297) ... 6 more This seems reasonable. For each test that checks vm.hasSAandCanAttach I also see. TEST RESULT: Error. Error evaluating _expression_: vm.hasSAandCanAttach: java.lang.RuntimeException: java.lang.InterruptedException This too seems reasonable. For tests that don't check vm.hasSAandCanAttach, but instead make a runtime check that calls Platform.shouldSAAttach(), the test fails with: java.lang.IllegalThreadStateException: process hasn't exited at java.base/java.lang.ProcessImpl.exitValue(ProcessImpl.java:500) at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380) at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:433) at ClhsdbAttach.main(ClhsdbAttach.java:77) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) This is a confusing way to fail. The reason it fails this way is because stopApp() first calls waitAppTerminiate(), which does the following: public void waitAppTerminate() { // This code is modeled after tail end of ProcessTools.getOutput(). try { appProcess.waitFor(); outPumperThread.join(); errPumperThread.join(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); // pass } } I added an e.printStackTrace() call and see the following: java.lang.InterruptedException at java.base/java.lang.Object.wait(Native Method) at java.base/java.lang.Object.wait(Object.java:321) at java.base/java.lang.ProcessImpl.waitFor(ProcessImpl.java:474) at jdk.test.lib.apps.LingeredApp.waitAppTerminate(LingeredApp.java:239) at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:380) at jdk.test.lib.apps.LingeredApp.stopApp(LingeredApp.java:434) So the earlier call to interrupt() is resulting in waitAppTerminate() not actually waiting for exit. This then results in stopApp() getting IllegalThreadStateException when calling Process.exitValue(). If I comment out the call to interrupt() in Platform.shouldSAAttach(), I think the failure stack trace is much better: java.lang.RuntimeException: Test ERROR java.lang.RuntimeException: java.lang.InterruptedException at ClhsdbAttach.main(ClhsdbAttach.java:75) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:832) Caused by: java.lang.RuntimeException: java.lang.InterruptedException at jdk.test.lib.Platform.shouldSAAttach(Platform.java:300) at ClhsdbLauncher.run(ClhsdbLauncher.java:199) at ClhsdbAttach.main(ClhsdbAttach.java:71) ... 6 more Caused by: java.lang.InterruptedException at jdk.test.lib.Platform.shouldSAAttach(Platform.java:297) ... 8 more There's still a minor issue with rethrowing the RuntimeException encapsulated inside another RuntimeException. That the fault of the test which is catching all Exceptions and encapsulating them in a RuntimeException, even if the Exceptions itself is already a RuntimeException. It should add have a catch clause for RuntimeException, and just rethrow it without encapulating it. All the Clhsdb tests seem to do this, so that's about 20 places to fix. Probably not worth doing unless some other cleanup is being done at the same time. Chris On 2/11/20 10:30 PM, Igor Ignatyev wrote: I'd say yes, it's better to still call Thread::interrupt. |
- Re: RFR: 8238196: tests that use SA Attach should not be... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatyev
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatyev
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatev
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatyev
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatyev
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... David Holmes
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... David Holmes
- Re: RFR: 8238196: tests that use SA Attach should n... Chris Plummer
- Re: RFR: 8238196: tests that use SA Attach should n... Igor Ignatyev
- Re: RFR: 8238196: tests that use SA Attach should n... serguei . spitsyn