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/classes/sun/tools/common/ProcessArgumentMatcher.java#l86 > > > > > > > >
