RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently
shanliang
shanliang.jiang at oracle.com
Wed Jul 2 08:05:37 UTC 2014
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
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list