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.plum...@oracle.com> > Sent: Freitag, 13. September 2019 19:49 > To: Langer, Christoph <christoph.lan...@sap.com>; Severin Gehwolf > <sgehw...@redhat.com>; Thomas Stüfe <thomas.stu...@gmail.com> > Cc: OpenJDK Serviceability <serviceability-dev@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 <sgehw...@redhat.com> > >> Sent: Donnerstag, 12. September 2019 11:01 > >> To: Langer, Christoph <christoph.lan...@sap.com>; Thomas Stüfe > >> <thomas.stu...@gmail.com> > >> Cc: OpenJDK Serviceability <serviceability-dev@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.stu...@gmail.com> > >>> Sent: Donnerstag, 12. September 2019 10:11 > >>> To: Langer, Christoph <christoph.lan...@sap.com> > >>> Cc: Chris Plummer <chris.plum...@oracle.com>; OpenJDK Serviceability > >> <serviceability-dev@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.lan...@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.plum...@oracle.com> > >>>> Sent: Mittwoch, 11. September 2019 19:21 > >>>> To: Thomas Stüfe <thomas.stu...@gmail.com>; Langer, Christoph > >> <christoph.lan...@sap.com> > >>>> Cc: OpenJDK Serviceability <serviceability-dev@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.lan...@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 > >>>> >
