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