Re: RFR JDK-8031554: com/sun/tools/attach/BasicTests.java fails intermittently

Tue, 01 Jul 2014 14:42:12 -0700 

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.
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.
About new failure, there could be a negative test which expected a IllegalStateException --> failed OutputAnalyser, who knows. 

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

Reply via email to