I agree with Jaroslav here, and would like a different patch. The proposed patch does not solve the problem and as a debugging help it is implemented at the wrong level. I think the ProcessTools.getOutput() should be updated to throw the InterruptedException - this looks wrong in the library. The impact on other tests should be minimal, I don’t believe anyone depends on getOutput() swallowing InterruptedException - if they do, we have to fix that, too (which at the same time would make those tests more stable).
Thanks, /Staffan On 2 jul 2014, at 10:05, shanliang <shanliang.ji...@oracle.com> wrote: > Jaroslav Bachorik wrote: >> On 07/01/2014 11:40 PM, shanliang wrote: >>> Jaroslav Bachorik wrote: >>>> Hi Shanliang, >>>> >>>> On 07/01/2014 09:47 PM, shanliang wrote: >>>>> I am still thinking to keep the original fix, because: >>>>> 1) to throw InterruptedException does not fix the test failure, it might >>>>> give more info for diagnostics. If it was really caused by an >>>>> InterruptedException, then to fix the issue we still need to know who >>>>> could interrupt the test main thread, in which case and why, and whether >>>>> possible to ignore it (skip the test or try again?). >>>> >>>> I'm sorry but I can't agree with this. The proposed patch does not add >>>> anything else than making the InterruptedException visible. Adding >>>> process.waitFor() will solve nothing as it is already called by >>>> ProcessTools.getOutput(process) while creating OutputAnalyzer. >>>> >>>>> 2) the test library is used also by other tests and to modify it might >>>>> make new fail, it is better to concentrate at first on a single test >>>>> before knowing the exact cause. >>>> >>>> I wouldn't expect new failures - when an InterruptedException was >>>> thrown the ProcessTools.executeTestJvm(args) returned null. Either the >>>> test would fail with NPE or custom assert - it makes no sense to work >>>> with "null" process. >>>> >>>> IMO, this should be fixed in the test library. >>> Sorry I may miss something here, you suggested: >>> "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." >>> >>> We still need to do something when an InterruptedException happens, skip >>> the test or retry? before making a decision we should know why there was >>> an InterruptedException and in which case, I really do not have any >>> idea, and I do not want to exclude other possibilities. >> >> The most probable explanation, based on the analysis of many intermittent >> test failures, is that the harness is trying to time out the test. But it is >> just an educated guess ... > Thought about this possibility, but a harness would kill the test instead to > interrupt the test thread? >> >>> >>> Yes what my fix does is to expose an InterruptedException if it happens, >>> but it could make sure that it was really because of an >>> InterruptedException. >> >> I don't feel particularly happy about the code duplication introduced by >> this fix. But if official reviewers are fine with it I won't block this >> review. >> >>> >>> About new failure, there could be a negative test which expected a >>> IllegalStateException --> failed OutputAnalyser, who knows. >> >> My concern is that there also might be other tests failing intermittently >> (or providing wrong results) due to the InterruptedException being silently >> swallowed. > I agree that we need to do investigation on the test library, but better to > do it later when we have more info from this test. > > Thanks, > Shanliang >> >> You might run the whole testsuite with the modified ProcessTools.getOutput() >> on JPRT and see if there are any negative tests failing (they should since >> instead of null value they will receive InterryptedException). >> >> -JB- >> >>> >>> Shanliang >>>> >>>> -JB- >>>> >>>>> >>>>> Shanliang >>>>> >>>>> Christian Tornqvist wrote: >>>>>> >>>>>> I can’t remember if there was a reason for doing it like this, but it >>>>>> seems reasonable to not catch the InterruptedException in getOutput(). >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Christian >>>>>> >>>>>> *From:*Staffan Larsen [mailto:staffan.lar...@oracle.com] >>>>>> *Sent:* Friday, June 27, 2014 4:49 AM >>>>>> *To:* shanliang >>>>>> *Cc:* Jaroslav Bachorik; serviceability-dev@openjdk.java.net >>>>>> serviceability-dev@openjdk.java.net; Christian Tornqvist >>>>>> *Subject:* Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java >>>>>> fails intermittently >>>>>> >>>>>> 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 >>>>>> <mailto: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/ >>>>>> <http://cr.openjdk.java.net/%7Esjiang/JDK-8031554/00/> >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Shanliang
