On Thu, 2019-09-12 at 19:53 +0000, 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.
Wow, that's unfortunate. > I would like to go with this version (changed the comment to Thomas' > suggestion): http://cr.openjdk.java.net/~clanger/webrevs/8230850.1/ Patch seems OK in this case. Thanks, Severin > 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
