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