It does look suspicious to catch and ignore the InterruptedException, especially since the OutputAnalyzer constructor will fail in this case.
Christian is the original author of these classes: do you know if there is a good rationale for doing this? Or should we propagate the InterruptedException? Thanks, /Staffan On 26 jun 2014, at 17:24, shanliang <shanliang.ji...@oracle.com> wrote: > Jaroslav Bachorik wrote: >> Hi Shanliang, >> >> On 06/26/2014 03:15 PM, shanliang wrote: >>> Hi, >>> >>> Today ProcessTools.executeProcess has the code: >>> new OutputAnalyzer(pb.start()); >>> >>> and OutputAnalyzer constructor calls immediately: >>> exitValue = process.exitValue(); >>> >>> the test got exception because the process did not end. >> >> Are you sure about this? >> >> The OutputAnalyzer constructor, before calling process.exitValue(), calls >> ProcessTools.getOutput(process) which actually does process.waitFor() > Good catch! >> >> A probable explanation would be that process.waitFor() gets interrupted >> without the target process actually ending. >> >> Either the result of ProcessTools.getOutput() should be checked for null to >> detect this situation or ProcessTools.getOutput() should throw a more >> aggressive exception when waitFor() gets interrupted. > It was possible beacause of an InterruptedException, but maybe a Process > issue too. > > process.waitFor() was called by the test main thread, I am wondering who > wanted to interrupt this thread? > > Not know why ProcessTools.getOutput() catches InterruptedException and then > returns null, are there some other tests dependent to this behavior? > otherwise better to not catch InterruptedException. > > I think to keep this modification, it will allow us to get more information > if the bug is reproduced, if getting an InterruptedException then we need to > do more investigation for why, if no exception then we may rise a question to > process.waitFor(). > > Shanliang >> >> -JB- >> >>> >>> So a direct solution for the test is not to call: >>> ProcessTools.executeTestJvm(args); >>> >>> but: >>> ProcessBuilder pb = >>> ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args)); >>> Process process = pb.start(); >>> process.waitFor(); >>> OutputAnalyzer output = new OutputAnalyzer(process); >>> >>> here we do waiting: >>> process.waitFor(); >>> before constructing an OutputAnalyzer. >>> >>> ProcessTools is a tool class we may have same issue for other tests >>> using this class. So we may need to improve the test library. >>> >>> bug: https://bugs.openjdk.java.net/browse/JDK-8031554 >>> webrev: http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/ >>> >>> >>> Thanks, >>> Shanliang
