RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently
Staffan Larsen
staffan.larsen at oracle.com
Wed Jul 2 11:32:20 UTC 2014
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.jiang at 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.larsen at oracle.com]
>>>>>> *Sent:* Friday, June 27, 2014 4:49 AM
>>>>>> *To:* shanliang
>>>>>> *Cc:* Jaroslav Bachorik; serviceability-dev at openjdk.java.net
>>>>>> serviceability-dev at 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.jiang at oracle.com
>>>>>> <mailto:shanliang.jiang at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140702/ed6af1bd/attachment-0001.html>
More information about the serviceability-dev
mailing list