[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8205654: serviceability/dcmd/framework/HelpTest.java timed out
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2019-01-31 21:23:28
Message-ID: 9cfcd666-e114-b508-d253-fb89b7cdc83b () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Daniil,<br>
<br>
<br>
I have some secondary comment on new file:<br>
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/src/jdk.jcmd/linux/classes/ \
sun/tools/ProcessHelper.java.html">http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java.html</a><br>
<br>
<br>
<pre> 70 for (int i = 0; i < parts.length && mainClass == \
null; i++) { 71 // Check the executable
72 if (i == 0) {
73 String[] executablePath = parts[i].split("/");
74 if (executablePath.length > 0) {
75 String binaryName = executablePath[executablePath.length - \
1]; 76 if (!"java".equals(binaryName)) {
77 // Skip the process if it is not started with java \
launcher 78 return null;
79 }
80 }
81 continue;
82 }</pre>
<br>
It is better execute the logic in lines 73-80 before the loop.<br>
It will simplify the code a little bit.<br>
<br>
<pre> String[] executablePath = parts[i].split("/");
if (executablePath.length > 0) {
String binaryName = executablePath[executablePath.length - 1];
if (!binaryName.equals("java") {
return null; // Skip the process if it is not started with java launcher
}
}
for (int i = 1; i < parts.length && mainClass == null; i++) {
In the fragment below:
83 // Check if the module is executed with explicitly specified main \
class 84 if ((parts[i].equals("-m") || parts[i].equals("--module")) \
&& i < parts.length - 1) { 85 mainClass = \
getMainClassFromModuleArg(parts[i + 1]); 86 break;
87 }
would it better to just return the main class instead of having a break statement? \
: 85 return getMainClassFromModuleArg(parts[i + 1]);
The lines:
108 if (jarFile != null) {
109 return getMainClassFromJar(jarFile, pid);
110 }
is better to execute inside the loop the same as it is done for \
getMainClassFromModuleArg().
// Check if the main class needs to be read from the manifest.mf in a JAR \
file if (parts[i].equals("-jar") && i < parts.length - 1) {
return getMainClassFromJar(jarFile, pid);
}
In the if statements:
84 if ((parts[i].equals("-m") || parts[i].equals("--module")) \
&& i < parts.length - 1) {
...
93 if (parts[i].equals("-jar") && i < parts.length - 1) {
the last condition (i < parts.length - 1) is better to make the first \
(pre-condition). They even can be combined together like below:
if (i < parts.length - 1) {
if ((parts[i].equals("-m") || parts[i].equals("--module"))) {
return getMainClassFromModuleArg(parts[i + 1]);
}
// Check if the main class needs to be read from the manifest.mf in a JAR file
if (parts[i].equals("-jar") ) {
return getMainClassFromJar(jarFile, pid);
}
}
</pre>
<br>
The biggest concern are the fragments:<br>
<pre> 88 if (parts[i].equals("-p") || \
parts[i].equals("--module-path")) { 89 i++;
90 continue;
91 }
...
97 // If this is a classpath option then skip the next part as well ( \
the classpath itself) 98 if (parts[i].equals("-cp") || \
parts[i].equals("-classpath")) { 99 i++;
100 continue;
101 }
102 // Skip all other Java options
103 if (parts[i].startsWith("-")) {
104 continue;
105 }
</pre>
If I understand it correctly, these statements are needed to
filter out<br>
the parts which have nothing to do with the mainClass.<br>
The latest part[i] that was not filtered out is returned as the
mainClass.<br>
<br>
I'm thinking about more general approach here. Probably, we just
need to remove<br>
the fragments 88-91 and 97-101 as they are covered by the
fragment 102-105.<br>
It will also simplify the code.<br>
<br>
With all the suggestion above it should converge to something
like this:<br>
<pre> String[] parts = cmdLine.split(" ");
String mainClass = null;
String[] executablePath = parts[i].split("/");
if (executablePath.length > 0) {
String binaryName = executablePath[executablePath.length - 1];
if (!binaryName.equals("java") {
return null; // Skip the process if it is not started with java launcher
}
}
for (int 1 = 0; i < parts.length && mainClass == null; i++) {
if (i < parts.length - 1) {
if ((parts[i].equals("-m") || parts[i].equals("--module"))) {
return getMainClassFromModuleArg(parts[i + 1]);
}
// Check if the main class needs to be read from the manifest.mf in a JAR \
file if (parts[i].equals("-jar") ) {
return getMainClassFromJar(parts[i + 1], pid);
}
}
// Skip all other Java options
if (parts[i].startsWith("-")) {
continue;
}
mainClass = parts[i];
}
return mainClass;
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/test/hotspot/jtreg/servicea \
bility/dcmd/framework/TestJavaProcess.java.html">http://cr.openjdk.java.net/~dtitov/82 \
05654/webrev.03/test/hotspot/jtreg/serviceability/dcmd/framework/TestJavaProcess.java.html</a>
49 if (cmd.equals("quit")) {
50 log("'quit' received");
51
52 } else {
The empty line 51 can be removed.
</pre>
<br>
Looking at this command-line processing I kind of understand the
David's concern.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 1/20/19 21:18, David Holmes wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c3d7d91b-1a26-bb3f-0b18-72c836475f53@oracle.com">Thanks
for the update Daniil. I still remain concerned about the
robustness of the command-line parsing - this seems like a feature
that needs its own set of tests.
<br>
<br>
I'll leave it up to Serguei and others as to how to proceed.
<br>
<br>
David
<br>
-----
<br>
<br>
On 19/01/2019 9:08 am, Daniil Titov wrote:
<br>
<blockquote type="cite">Hi David and Serguei,
<br>
<br>
Please review a new version of the fix that now covers the case
when Java executes a module with the main class name explicitly
specified in the command line.
<br>
<br>
Webrev: <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8205654/webrev.03">http://cr.openjdk.java.net/~dtitov/8205654/webrev.03</a>
<br>
Bug: : <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8205654">https://bugs.openjdk.java.net/browse/JDK-8205654</a>
<br>
<br>
Thanks!
<br>
--Daniil
<br>
<br>
On 1/8/19, 6:05 PM, "David Holmes"
<a class="moz-txt-link-rfc2396E" \
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a> wrote: \
<br> <br>
Hi Daniil,
<br>
Sorry this slipped through the Xmas break cracks :)
<br>
On 22/12/2018 12:04 pm, Daniil Titov wrote:
<br>
> Hi David and Serguei,
<br>
>
<br>
> Please review a new version of the fix that for Linux
platform uses the proc filesystem to retrieve the main class
name for the running Java process.
<br>
>
<br>
> Webrev:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8205654/webrev.02/">http://cr.openjdk.java.net/~dtitov/8205654/webrev.02/</a>
<br>
> Bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8205654">https://bugs.openjdk.java.net/browse/JDK-8205654</a>
<br>
It's more complex than I had envisaged but seems to be
doing the job.
<br>
I'm not sure how robust the command-line parsing is, in
particular it
<br>
doesn't handle these forms:
<br>
or java [options] -m
<module>[/<mainclass>] [args...]
<br>
java [options] --module
<module>[/<mainclass>] [args...]
<br>
(to execute the main class in a module)
<br>
I can't really comment on all the details.
<br>
Thanks,
<br>
David
<br>
-----
<br>
> Thanks,
<br>
> Daniil
<br>
>
<br>
> On 11/29/18, 4:52 PM, "David Holmes"
<a class="moz-txt-link-rfc2396E" \
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a> wrote: \
<br> >
<br>
> Hi Daniil,
<br>
>
<br>
> On 30/11/2018 7:30 am, Daniil Titov wrote:
<br>
> > Thank you, David!
<br>
> >
<br>
> > The proposed fix didn't help. It still hangs
at some occasions. Additional tracing showed that when jcmd is
invoked with the main class name it iterates over all running
Java processes and temporary attaches to them to retrieve the
main class name. It hangs while trying to attach to one of the
running Java processes. There are numerous Java processes
running at the host machine some associated with the test
framework itself and another with the tests running in parallel.
It is not clear what exact is this particular process since the
jcmd hangs before retrieving the process' main class name, but
after all tests terminated the process with this id is no longer
running. I have to revoke this review since more investigation
is required.
<br>
>
<br>
> That sounds like an unsolvable problem for the
test. You can't control
<br>
> other Java processes on the machine, and
searching by name requires
<br>
> asking each of them in turn.
<br>
>
<br>
> How do we get the list of Java processes in the
first place? Perhaps we
<br>
> need to do some /proc/<pid>/cmdline
peeking?
<br>
>
<br>
> Cheers,
<br>
> David
<br>
>
<br>
> >
<br>
> > Best regards,
<br>
> > Daniil
<br>
> >
<br>
> >
<br>
> >
<br>
> > On 11/11/18, 1:35 PM, "David Holmes"
<a class="moz-txt-link-rfc2396E" \
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a> wrote: \
<br> > >
<br>
> > Hi Daniil,
<br>
> >
<br>
> > I took a quick look at this one ... \
two minor comments
<br>
> >
<br>
> > The static class names could just be
"Process" as they will acquire the
<br>
> > enclosing class name as part of their
own name anyway. As it is this
<br>
> > gets repeated eg:
<br>
> >
<br>
> > HelpTest$HelpTestProcess
<br>
> >
InvalidCommandTest$InvalidCommandTestProcess
<br>
> >
<br>
> > TestJavaProcess.java:
<br>
> >
<br>
> > 39 public static void \
main(String argv[]) {
<br>
> >
<br>
> > Nit: Should be "String[] argv" in Java
style
<br>
> >
<br>
> > Thanks,
<br>
> > David
<br>
> >
<br>
> > On 10/11/2018 3:18 PM, Daniil Titov
wrote:
<br>
> > > Please review the change that
fixes serviceability/dcmd/framework/* tests from a time out. The
fix for JDK-8166642 made serviceability/dcmd/framework/* tests
non-concurrent to ensure that they don't interact with each
other and there are no multiple tests running simultaneously
since all they do share the common main class name
com.sun.javatest.regtest.agent.MainWrapper. However, it looks
like the tests from other directories still might run in
parallel with these tests and they also have
com.sun.javatest.regtest.agent.MainWrapper as a main class.
<br>
> > >
<br>
> > > The fix ensures that each
serviceability/dcmd/framework/* test uses a Java process with a
unique main class name when connecting to this process with jcmd
and the main class name.
<br>
> > >
<br>
> > > Bug:
<a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8205654">https://bugs.openjdk.java.net/browse/JDK-8205654</a>
<br>
> > > Webrev:
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8205654/webrev.001/">http://cr.openjdk.java.net/~dtitov/8205654/webrev.001/</a>
<br>
> > >
<br>
> > > Best regards,
<br>
> > > Daniil
<br>
> > >
<br>
> > >
<br>
> >
<br>
> >
<br>
> >
<br>
>
<br>
>
<br>
>
<br>
<br>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic