[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently
From: Staffan Larsen <staffan.larsen () oracle ! com>
Date: 2014-07-02 11:32:20
Message-ID: 17463FCA-AAB0-42C4-804E-4AEA162BF0C8 () oracle ! com
[Download RAW message or body]
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@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@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.jiang@oracle.com
> > > > > > <mailto:shanliang.jiang@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
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: \
space; -webkit-line-break: after-white-space;">I agree with Jaroslav here, and would \
like a different patch.<div><br></div><div>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).<div><br></div><div><div>Thanks,</div><div>/Staffan</div><div><br><div><div>On \
2 jul 2014, at 10:05, shanliang <<a \
href="mailto:shanliang.jiang@oracle.com">shanliang.jiang@oracle.com</a>> \
wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div \
style="font-size: 16px; font-style: normal; font-variant: normal; font-weight: \
normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: \
start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; \
word-spacing: 0px; -webkit-text-stroke-width: 0px;">Jaroslav Bachorik \
wrote:<br><blockquote type="cite">On 07/01/2014 11:40 PM, shanliang \
wrote:<br><blockquote type="cite">Jaroslav Bachorik wrote:<br><blockquote \
type="cite">Hi Shanliang,<br><br>On 07/01/2014 09:47 PM, shanliang \
wrote:<br><blockquote type="cite">I am still thinking to keep the original fix, \
because:<br>1) to throw InterruptedException does not fix the test failure, it \
might<br>give more info for diagnostics. If it was really caused by \
an<br>InterruptedException, then to fix the issue we still need to know who<br>could \
interrupt the test main thread, in which case and why, and whether<br>possible to \
ignore it (skip the test or try again?).<br></blockquote><br>I'm sorry but I can't \
agree with this. The proposed patch does not add<br>anything else than making the \
InterruptedException visible. Adding<br>process.waitFor() will solve nothing as it is \
already called by<br>ProcessTools.getOutput(process) while creating \
OutputAnalyzer.<br><br><blockquote type="cite">2) the test library is used also by \
other tests and to modify it might<br>make new fail, it is better to concentrate at \
first on a single test<br>before knowing the exact cause.<br></blockquote><br>I \
wouldn't expect new failures - when an InterruptedException was<br>thrown the \
ProcessTools.executeTestJvm(args) returned null. Either the<br>test would fail with \
NPE or custom assert - it makes no sense to work<br>with "null" process.<br><br>IMO, \
this should be fixed in the test library.<br></blockquote>Sorry I may miss something \
here, you suggested:<br> "Either the result of \
ProcessTools.getOutput() should be checked for<br>null to detect this situation or \
ProcessTools.getOutput() should throw a<br>more aggressive exception when waitFor() \
gets interrupted."<br><br>We still need to do something when an InterruptedException \
happens, skip<br>the test or retry? before making a decision we should know why there \
was<br>an InterruptedException and in which case, I really do not have any<br>idea, \
and I do not want to exclude other possibilities.<br></blockquote><br>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 \
...<br></blockquote>Thought about this possibility, but a harness would kill the test \
instead to interrupt the test thread?<br><blockquote type="cite"><br><blockquote \
type="cite"><br>Yes what my fix does is to expose an InterruptedException if it \
happens,<br>but it could make sure that it was really because of \
an<br>InterruptedException.<br></blockquote><br>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.<br><br><blockquote type="cite"><br>About new failure, \
there could be a negative test which expected a<br>IllegalStateException --> \
failed OutputAnalyser, who knows.<br></blockquote><br>My concern is that there also \
might be other tests failing intermittently (or providing wrong results) due to the \
InterruptedException being silently swallowed.<br></blockquote>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.<br><br>Thanks,<br>Shanliang<br><blockquote type="cite"><br>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).<br><br>-JB-<br><br><blockquote \
type="cite"><br>Shanliang<br><blockquote type="cite"><br>-JB-<br><br><blockquote \
type="cite"><br>Shanliang<br><br>Christian Tornqvist wrote:<br><blockquote \
type="cite"><br>I can’t remember if there was a reason for doing it like this, but \
it<br>seems reasonable to not catch the InterruptedException in \
getOutput().<br><br>Thanks,<br><br>Christian<br><br>*From:*Staffan Larsen [<a \
href="mailto:staffan.larsen@oracle.com">mailto:staffan.larsen@oracle.com</a>]<br>*Sent:* \
Friday, June 27, 2014 4:49 AM<br>*To:* shanliang<br>*Cc:* Jaroslav Bachorik; <a \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br><a \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a>; \
Christian Tornqvist<br>*Subject:* Re: RFR JDK-8031554: \
com/sun/tools/attach/BasicTests.java<br>fails intermittently<br><br>It does look \
suspicious to catch and ignore the InterruptedException,<br>especially since the \
OutputAnalyzer constructor will fail in this case.<br><br>Christian is the original \
author of these classes: do you know if<br>there is a good rationale for doing this? \
Or should we propagate \
the<br>InterruptedException?<br><br>Thanks,<br><br>/Staffan<br><br>On 26 jun 2014, at \
17:24, shanliang <<a \
href="mailto:shanliang.jiang@oracle.com">shanliang.jiang@oracle.com</a><br><<a \
href="mailto:shanliang.jiang@oracle.com">mailto:shanliang.jiang@oracle.com</a>>> \
wrote:<br><br><br><br> Jaroslav Bachorik \
wrote:<br><br> Hi \
Shanliang,<br><br> On 06/26/2014 03:15 PM, \
shanliang wrote:<br><br> &n \
bsp;Hi,<br><br> Today \
ProcessTools.executeProcess has the \
code:<br> new \
OutputAnalyzer(pb.start());<br><br> and \
OutputAnalyzer constructor calls \
immediately:<br> exitValue \
= process.exitValue();<br><br> the \
test got exception because the process did not \
end.<br><br><br> Are you sure about \
this?<br><br> The OutputAnalyzer \
constructor, before calling<br> process.exitValue(), \
calls ProcessTools.getOutput(process)<br> which \
actually does process.waitFor()<br><br> Good \
catch!<br><br><br> A probable explanation \
would be that process.waitFor() \
gets<br> interrupted without the target \
process actually ending.<br><br> Either the \
result of ProcessTools.getOutput() should \
be<br> checked for null to detect this \
situation or<br> ProcessTools.getOutput() \
should throw a more aggressive<br> exception \
when waitFor() gets interrupted.<br><br> It was possible beacause of \
an InterruptedException, but maybe a<br> Process issue \
too.<br><br> process.waitFor() was called by the test main thread, I \
am<br> wondering who wanted to interrupt this \
thread?<br><br> Not know why ProcessTools.getOutput() catches \
InterruptedException<br> and then returns null, are there some other \
tests dependent to<br> this behavior? otherwise better to not catch \
InterruptedException.<br><br> I think to keep this modification, it \
will allow us to get more<br> information if the bug is reproduced, \
if getting an<br> InterruptedException then we need to do more \
investigation for<br> why, if no exception then we may rise a \
question to<br>process.waitFor().<br><br> Shanliang<br><br><br> \
-JB-<br><br><br><br> So \
a direct solution for the test is not to \
call:<br>   \
; ProcessTools.executeTestJvm(args);<br><br> & \
nbsp; but:<br> & \
nbsp; ProcessBuilder \
pb =<br>ProcessTools.createJavaProcessBuilder(Utils.addTestJavaOpts(args));<br> & \
nbsp; Process \
process = pb.start();<br> & \
nbsp; process.waitFor();<br>   \
; OutputAnalyzer \
output = new<br>OutputAnalyzer(process);<br><br> here \
we do waiting:<br> &n \
bsp; process.waitFor();<br> before \
constructing an OutputAnalyzer.<br><br> ProcessTools \
is a tool class we may have same issue \
for<br> other \
tests<br> using this \
class. So we may need to improve the \
test<br>library.<br><br> bug: \
<a href="https://bugs.openjdk.java.net/browse/JDK-8031554">https://bugs.openjdk.java.n \
et/browse/JDK-8031554</a><br> webrev: \
<a href="http://cr.openjdk.java.net/~sjiang/JDK-8031554/00/">http://cr.openjdk.java.ne \
t/~sjiang/JDK-8031554/00/</a><br> <<a \
href="http://cr.openjdk.java.net/%7Esjiang/JDK-8031554/00/">http://cr.openjdk.java.net \
/%7Esjiang/JDK-8031554/00/</a>><br><br><br> &nbs \
p; Thanks,<br> & \
nbsp; Shanliang</blockquote></blockquote></blockquote></blockquote></blockquote></div></blockquote></div><br></div></div></div></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic