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

Thu Sep 12 19:53:18 UTC 2019

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
> > > > >
> > >
> > >