Hi Daniil,
Thank you for the update!
It looks good in general.
I think, it can be a good idea to add a simple tests for this
command-line processing.
It should save us from any surprises.
Thanks,
Serguei
On 2/4/19 13:24, Daniil Titov wrote:
Hi Serguei,
Thank you for reviewing this fix.
Please review a new version of the fix that includes all changes you suggested
but one about lines 88-91 and 97-101.
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 }
You are right, these statements are needed to filter out the parts which have
nothing to do with the mainClass. But we cannot remove these lines and just
return the latest part[i] that was not filtered out as the mainClass since it will
not work in the case when the command line includes arguments specified after the
main class. In the approach we use the main class is the *first* part[i] (with i
> 0) that is not a Java option ( part[i] that doesn't start with '-' and is not
classpath, module path, jar file path or module name). This condition that the
mainClass is not null is checked on line 89 inside for loop.
89 for (int i = 1; i < parts.length && mainClass == null; i++) {
To simplify the code in the new version of the patch the lines 88-91 and 97-101 are
combined in the one "if" block.
Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.04
Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
Thanks.
--Daniil
From: "[email protected]" <[email protected]>
Date: Thursday, January 31, 2019 at 1:23 PM
To: David Holmes <[email protected]>, Daniil Titov <[email protected]>,
serviceability-dev <[email protected]>
Subject: Re: RFR 8205654: serviceability/dcmd/framework/HelpTest.java timed out
Hi Daniil,
I have some secondary comment on new file:
http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/src/jdk.jcmd/linux/classes/sun/tools/ProcessHelper.java.html
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 }
It is better execute the logic in lines 73-80 before the loop.
It will simplify the code a little bit.
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);
}
}
The biggest concern are the fragments:
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 }
If I understand it correctly, these statements are needed to filter out
the parts which have nothing to do with the mainClass.
The latest part[i] that was not filtered out is returned as the mainClass.
I'm thinking about more general approach here. Probably, we just need to
remove
the fragments 88-91 and 97-101 as they are covered by the fragment 102-105.
It will also simplify the code.
With all the suggestion above it should converge to something like this:
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;
http://cr.openjdk.java.net/~dtitov/8205654/webrev.03/test/hotspot/jtreg/serviceability/dcmd/framework/TestJavaProcess.java.html
49 if (cmd.equals("quit")) {
50 log("'quit' received");
51
52 } else {
The empty line 51 can be removed.
Looking at this command-line processing I kind of understand the David's
concern.
Thanks,
Serguei
On 1/20/19 21:18, David Holmes wrote:
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.
I'll leave it up to Serguei and others as to how to proceed.
David
-----
On 19/01/2019 9:08 am, Daniil Titov wrote:
Hi David and Serguei,
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.
Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.03
Bug: : https://bugs.openjdk.java.net/browse/JDK-8205654
Thanks!
--Daniil
On 1/8/19, 6:05 PM, "David Holmes" mailto:[email protected] wrote:
Hi Daniil,
Sorry this slipped through the Xmas break cracks :)
On 22/12/2018 12:04 pm, Daniil Titov wrote:
> Hi David and Serguei,
>
> 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.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8205654/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
It's more complex than I had envisaged but seems to be doing the job.
I'm not sure how robust the command-line parsing is, in particular it
doesn't handle these forms:
or java [options] -m <module>[/<mainclass>] [args...]
java [options] --module <module>[/<mainclass>] [args...]
(to execute the main class in a module)
I can't really comment on all the details.
Thanks,
David
-----
> Thanks,
> Daniil
>
> On 11/29/18, 4:52 PM, "David Holmes" mailto:[email protected]
wrote:
>
> Hi Daniil,
>
> On 30/11/2018 7:30 am, Daniil Titov wrote:
> > Thank you, David!
> >
> > 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.
>
> That sounds like an unsolvable problem for the test. You can't
control
> other Java processes on the machine, and searching by name requires
> asking each of them in turn.
>
> How do we get the list of Java processes in the first place?
Perhaps we
> need to do some /proc/<pid>/cmdline peeking?
>
> Cheers,
> David
>
> >
> > Best regards,
> > Daniil
> >
> >
> >
> > On 11/11/18, 1:35 PM, "David Holmes"
mailto:[email protected] wrote:
> >
> > Hi Daniil,
> >
> > I took a quick look at this one ... two minor comments
> >
> > The static class names could just be "Process" as they will
acquire the
> > enclosing class name as part of their own name anyway. As
it is this
> > gets repeated eg:
> >
> > HelpTest$HelpTestProcess
> > InvalidCommandTest$InvalidCommandTestProcess
> >
> > TestJavaProcess.java:
> >
> > 39 public static void main(String argv[]) {
> >
> > Nit: Should be "String[] argv" in Java style
> >
> > Thanks,
> > David
> >
> > On 10/11/2018 3:18 PM, Daniil Titov wrote:
> > > 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.
> > >
> > > 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.
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8205654
> > > Webrev:
http://cr.openjdk.java.net/~dtitov/8205654/webrev.001/
> > >
> > > Best regards,
> > > Daniil
> > >
> > >
> >
> >
> >
>
>
>
