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

 


Reply via email to