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