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