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

Sun Sep 15 05:57:15 UTC 2019

Hi Chris,

I guess it's the multitude of JVMs that get started one after each other and not in parallel.

So I'll push my fix then.

Best regards
Christoph

> -----Original Message-----
> From: Chris Plummer <chris.plummer at oracle.com>
> Sent: Freitag, 13. September 2019 19:49
> To: Langer, Christoph <christoph.langer at sap.com>; Severin Gehwolf
> <sgehwolf at redhat.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
> 
> 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
> >>>>
> 