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

[Severin Gehwolf]

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/classes/sun/tools/common/ProcessArgumentMatcher.java#l86
> > > >  
> > 
> >