RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java fails intermittently

Fri Sep 13 17:49:00 UTC 2019

3 minutes??? Sounds like something is wrong. What is the JVM doing 
during this time?

Chris

On 9/12/19 12:53 PM, Langer, Christoph wrote:
> Hi Severin,
>
> that seems an interesting idea for an elegant solution. However, after trying this on a decently fast linux x86 box by leveraging one of these ProcessTools::startProcess methods that would wait for a certain output to appear in the child before returning, I figured that the elapsed runtime of the test increases from about 3 seconds to 3 minutes. It evidently takes a lot longer to bootstrap a JVM and get the results of a first println than just forking a process and immediately accessing its proc filesystem. So I think we don't want to do that.
>
> I would like to go with this version (changed the comment to Thomas' suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/
>
> Chris, are you ok with it?
>
> Thanks
> Christoph
>
>> -----Original Message-----
>> From: Severin Gehwolf <sgehwolf at redhat.com>
>> Sent: Donnerstag, 12. September 2019 11:01
>> To: Langer, Christoph <christoph.langer at sap.com>; Thomas Stüfe
>> <thomas.stuefe at gmail.com>
>> Cc: OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
>> Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
>> fails intermittently
>>
>> Hi Christoph,
>>
>> Have you considered to wait for TestProcess - the spawned processes - to
>> print this on stdout:
>>
>> "The process started, pid: XXX"
>>
>> Once that's ready on stdout, checking the main class should always
>> pass. I believe p.isAlive() check which is currrently done is
>> insufficient.
>>
>> Thanks,
>> Severin
>>
>> On Thu, 2019-09-12 at 08:12 +0000, Langer, Christoph wrote:
>>> Hi Thomas,
>>>
>>> sounds reasonable, will do.
>>>
>>> Thanks
>>> Christoph
>>>
>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>> Sent: Donnerstag, 12. September 2019 10:11
>>> To: Langer, Christoph <christoph.langer at sap.com>
>>> Cc: Chris Plummer <chris.plummer at oracle.com>; OpenJDK Serviceability
>> <serviceability-dev at openjdk.java.net>
>>> Subject: Re: RFR (S): 8230850: Test sun/tools/jcmd/TestProcessHelper.java
>> fails intermittently
>>> I'm fine with the patch if you would reshape the platform dependent
>> comment. Proposal:
>>> ----
>>> -        // Depending on hw/os, process helper can return null here
>>> -        // because /proc/<pid>/cmdline is not ready yet. To cover that case,
>>>          // give it some retries.
>>> ->
>>> + getMainClass() may return NULL, e.g. due to timing issues. Attempt some
>> limited retries.
>>> ----
>>> I do not need another webrev.
>>> Cheers, Thomas
>>>
>>>
>>>
>>>
>>> On Wed, Sep 11, 2019 at 11:37 PM Langer, Christoph
>> <christoph.langer at sap.com> wrote:
>>>> Hi Chris, Thomas,
>>>>
>>>> thanks for looking at this. I was also wondering whether a fix in
>> ProcessHelper would be appropriate. But I think introducing retries and
>> delays in that code can do more harm than help.
>>>> For this special test case, aiming to test the ProcessHelper functionality
>> (on Linux) only, the observed problem is that the /proc/<pid>/cmdline file is
>> not ready yet when it gets evaluated because the test can be quicker than
>> the spawned processes. But in real life usage of jcmd this seems rather
>> unlikely. One will probably use jcmd quite some time after a java process was
>> started and /proc/<pid>/cmdline should be ready. If then there are
>> problems reading it, there are likely other issues which won’t go away by
>> waiting. And for these cases the fallback is to use the attach framework, as
>> implemented in ProcessArgumentMatcher, which provides some chance to
>> be working still. And this fallback should also cover the exotic case when jcmd
>> is issued too early.
>>>> After all, ProcessHelper::getMainClass also documents that its result can
>> be null.
>>>> @Thomas, as for your other points:
>>>>   PID reusage: Hm, maybe one can construct cases. However, I’d think the
>> /proc/pid files should be gone after a process ends. Or at least be
>> reconstructed if there were orphans and a new process reusing an old pid
>> gets started. But who knows what can happen – we’ll maybe see ��
>>>>   Comment for Linux only issue: The test is in fact a Linux only test. See line
>> 55: * @requires os.family == "linux". So, if we’ll eventually see
>> implementations for ProcessHelper::getMainClass on other platforms, this
>> comment might have to be adopted. But for the time being I guess it’s fine at
>> its current place.
>>>> Would you agree?
>>>>
>>>> Best regards
>>>> Christoph
>>>>
>>>> From: Chris Plummer <chris.plummer at oracle.com>
>>>> Sent: Mittwoch, 11. September 2019 19:21
>>>> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Langer, Christoph
>> <christoph.langer at sap.com>
>>>> Cc: OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
>>>> Subject: Re: RFR (S): 8230850: Test
>> sun/tools/jcmd/TestProcessHelper.java fails intermittently
>>>> It does seem that the fix should be in ProcessHelper.java in
>> getMainClass(), or maybe even getCommandLine(). Fixing it in the test
>> implies that every user of getMainClass() should be doing something similar.
>> But then also note what ProcessArgumentMatch.check() is doing. It also
>> deals with getMainClass() returing null.
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 9/11/19 6:59 AM, Thomas Stüfe wrote:
>>>>> Hi Christoph,
>>>>>
>>>>> in general I think this is fine. The increase-by-pow2 sleep time is odd
>> but okay :)
>>>>> The whole things seems rather fragile and has a lot of question marks
>> but I think your fix does not make it worse. One fun error now is that with a
>> follow up java test reusing the PID we could get a wrong main class but I think
>> the chances are astronomically low.
>>>>> Only remark, you fix this in the platform shared code, if this is a Linux
>> only issue maybe it should be fixed in /shared/projects/openjdk/jdk-
>> jdk/source/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java
>> instead? If not, I would remove at least the /proc/<pid>/cmdline comment
>> since this is quite platform specific.
>>>>> Cheers, Thomas
>>>>>
>>>>>
>>>>> On Wed, Sep 11, 2019 at 2:39 PM Langer, Christoph
>> <christoph.langer at sap.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> please review this change for test
>> sun/tools/jcmd/TestProcessHelper.java to make it more robust.
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230850
>>>>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8230850.0/
>>>>>>
>>>>>> This Linux only test is starting several Java processes and then tries to
>> figure out the main class by invoking jdk.jcmd's linux specific ProcessHelper
>> implementation  which parses the contents of /proc/<pid>/cmdline.
>>>>>> Under some circumstances, the test already attempts to read
>> /proc/<pid>/cmdline before it actually exists or is filled with data. This can be
>> fixed with some sleeps/retries to wait for that data to be ready.
>>>>>> In the actual jcmd tool, such behavior of ProcessHelper. getMainClass
>> should not be an issue because it is handled in ProcessArgumentMatcher [0].
>>>>>> Thanks
>>>>>> Christoph
>>>>>>
>>>>>> [0]
>> http://hg.openjdk.java.net/jdk/jdk/file/8b08eaf9a0eb/src/jdk.jcmd/share/cl
>> asses/sun/tools/common/ProcessArgumentMatcher.java#l86
>>>>