Hi,
Please take a look at the new review.
Webrev for jdk part:
http://cr.openjdk.java.net/~anazarov/7195249/jdk/webrev.02/
Webrev for hs part:
http://cr.openjdk.java.net/~anazarov/7195249/hs/webrev.02/
My answers are inline:
On 08.01.2014 17:46, Staffan Larsen wrote:
Hi Taras,
Thanks for doing this clean up and conversion of tests into Java.
Here’s a couple of comments:
test/runtime/6294277/SourceDebugExtension.java:
This test could be simplified by not specifying an address at all.
Since the test never connects to the JVM started with -Xrunjdwp, there
is no reason to specify an address. If address is unspecified (and
server=y), the connector will pick an address and print it to the
command line. Thus the only change that needs to be done is to remove
",address=8888” from the @run command.
fixed
test/sun/management/jmxremote/bootstrap/RmiBootstrapTest.sh:
test/sun/management/jmxremote/bootstrap/RmiSslBootstrapTest.sh:
These tests do not compile cleanly with an empty JTwork directory. It
seems that having one @build for each class does not work well - when
compiling RmiBootstrapTest.java it cannot find TestLogger. Moving all
classes to one @build statement solved this problem for me.
fixed
test/lib/testlibrary/jdk/testlibrary/ProcessTools.java:
187 Future<Void> stdoutTask = stdout.process();
188 Future<Void> stderrTask = stderr.process();
The stdoutTask and stderrTask variables are unused.
fixed
test/sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java:
At first I thought something was wrong with this file - the diff is
very weird. Then I realized you renamed an old file and created a new
file using the old name.
You are right. I did it to keep the test name.
test/sun/management/jmxremote/bootstrap/AbstractFilePermissionTest.java:
- Is resetPasswordFilePermission() really necessary? It looks like you
delete the files at the beginning of the test in any case.
I think yes. n the first place, this functionality was at the old code.
In the second place, a file without write permission may be a problem
for a further cleanup (not by the test, for example for the tests
launcher scripts etc.)
- I find the names and usage of “mgmt” and “file2PermissionTest”
confusing. They are both Paths. One is used directly by the
sub-classes, the other has a getter method.
fixed
- Lines 57-58: Don’t swallow exceptions, add an ex.printStackTrace().
(Same thing for all other places where you call Integer.parseInt())
fixed
test/sun/management/jmxremote/bootstrap/Dummy.java:
This file is never used as far as I can see.
It is used by PasswordFilePermissionTest & SSLConfigFilePermissionTest
via the AbstractFilePermissionTest (see the doTest method,
AbstractFilePermissionTest : 162).
Thanks,
/Staffan
On 26 dec 2013, at 14:09, taras ledkov <taras.led...@oracle.com>
wrote:
Hi,
Please take a look at the review with fixed issues about trying to
launch test that needs free port several times.
Webrev for jdk part:
http://cr.openjdk.java.net/~anazarov/7195249/jdk/webrev.01/
Webrev for hs part:
http://cr.openjdk.java.net/~anazarov/7195249/hs/webrev.01/
Pay your attention to new method ProcessTools.startProcess(String,
ProcessBuilder, Consumer<String>) that is used to analyze all output
of a sub-process. It has common part with
ProcessTools.startProcess(String, ProcessBuilder, Predicate<String>,
long, TumeUnit) that is used to determine the warm-up moment.
I think the ProcessTools.startProcess(String, ProcessBuilder,
Predicate<String>, long, TumeUnit) may be changed by adding LinePump
to stderr if there is not serious reason for restricting the warm-up
analysis to stdout stream.
On 10.12.2013 16:16, Yekaterina Kantserova wrote:
Hi,
I've consulted with Serviceability engineers (add them to CC
list) and
they would like to see tests to solve these problem so far:
2. Implement loops in every test.
Thanks,
Katja
On 12/09/2013 11:02 AM, Alexandre (Shura) Iline wrote:
Guys.
Let me try to sum up what was said before and may be suggest a
compromise.
1. There is a desire to have a support port allocation on the
level of
a JTReg suite execution. Taras created a bug for that
(https://bugs.openjdk.java.net/browse/JDK-7195249). Whether it is a
test harness API or a library API does not really matter from usage
point of view.
2. There is no way to make the tests absolutely stable, whatever
port
allocation logic is used. The best we could do is to try to perform
the test logic with different ports until the test succeeds.
Both arguments make sense. #2 is the ultimate answer, of course,
but
better be used in conjunction with a meaningful port selection
algorithm.
At the same time, copying a loop-until-success login from one
test to
another may be not the best solution. Library could help with
that I
believe. There only need to be an API method which takes
behavior as a
parameter and run it until it succeeds. Something like:
public <T> runOnAFreePort(Function<T, Integer>)
or similar. There could be arguments of how/whether to implement
it,
the solution would not work for shell tests, etc, but still ...
With the tests in question though, we have a few options.
1. Integrate tests as is. Get to it later after reaching
agreement in
the library, etc.
2. Implement loops in every test.
3. Wait for the library to be ready and only then integrate the
changes.
Please let us know which one is closer to your heart.
I personally prefer #1 for the reason that the changes already
supposed to make the tests more stable and also there are many more
tests tests which use ports, so the scope of the problem is bigger
than these.
Shura
Taras,
I agree with the previous comments, that Utils.getFreePort()
does not
guarantee the port will be still free when you start your process.
Unfortunately I don't think the library can do more. However,
there is a
solution.
Please, look at the *jdk/test/sun/tools/jstatd/JstatdTest.java
tryToSetupJstatdProcess()*. In brief, the test will try to start a
process with a free port and then check if
/java.rmi.server.ExportException: Port already in use/ has been
thrown.
If yes, you have to retry.
Thanks,
Katja
On 12/02/2013 01:39 PM, taras ledkov wrote:
Hi Everyone,
Whatever logic is to be chosen to select a free port, it is the
library responsibility to implements it, would not you agree?
Hence what I am suggesting is to integrate the tests as is.
Should we decide to replace logic of the port selection, we
could do
it later in the library.
On 21.11.2013 15:00, Jaroslav Bachorik wrote:
On 20.11.2013 18:38, Dmitry Samersoff wrote:
Roger,
As soon as we close a socket nobody can guarantee that the
port is
free.
Moreover, port returned by getFreePort()[1] remains not
accessible
for
some time - it depends to system setup, take a look to
discussions
around SO_REUSEPORT for Linux or SO_REUSEADDR and SO_LINGER for
BSD.
So from stability point of view it's better to just return
random
number
between 49152 and 65535.
Well, this doesn't seem to improve the odds by much. When
there are
more
tests run in parallel, all of them requiring a free port,
nothing
prevents the random function to return the same port to all of
them.
Also, two subsequent requests can return the same port and cause
problems with timing when a port used by a previous test is not
fully
ready to be assigned to a different socket. And as Dmitry
pointed out
unless one can keep hold of the allocated socket and use it
later
there
is no guarantee that a port which was tested unallocated will
remain
unallocated also for the next few milliseconds.
The only fail proof solution would be a port allocating service
provided
by the harness. Until then we can only (hopefully) decrease the
chance
of intermittent failures due to a port being in use.
-JB-
-Dmitry
[1]
141 public static int getFreePort() throws
InterruptedException,
IOException {
142 int port = -1;
143
144 while (port <= 0) {
145 Thread.sleep(100);
146
147 ServerSocket serverSocket = null;
148 try {
149 serverSocket = new ServerSocket(0);
150 port = serverSocket.getLocalPort();
151 } finally {
152 serverSocket.close();
153 }
154 }
155
156 return port;
157 }
158
On 2013-11-20 19:40, roger riggs wrote:
Hi,
fyi, The jdk.testlibrary.Utils.getFreePort() method will
Open an
free
Socket, close it and return
the port number.
And as Alan recommended, use (0) when possible to have the
system
assign
the port #.
Roger
On 11/20/2013 8:04 AM, Dmitry Samersoff wrote:
Taras,
*The only* correct way to take really free port is:
1. Chose random number between 49152 and 65535
2. Open socket
if socket fails - repeat step 1
if socket OK - return *socket*
If you can't keep the socket open (e.g. you have to pass port
number as
property value) you shouldn't do any pre-check as it has no
value
- as
as soon as you close socket someone can take the port.
So just choose a random number within the range above and let
networking
code opening socket to handle port conflict.
-Dmitry
On 2013-11-20 15:54, taras ledkov wrote:
Hi Everyone,
I am working on bug
https://bugs.openjdk.java.net/browse/JDK-7195249.
There are two webrevs:
Webrev for jdk part:
http://cr.openjdk.java.net/~anazarov/7195249/jdk/webrev.00/
Webrev for hs part:
http://cr.openjdk.java.net/~anazarov/7195249/hs/webrev.00/
Please take a look at some notes:
- After discussing with Yekaterina Kantserova & Jaroslav
Bachorik
some
shell tests have been converted to java based tests
- PasswordFilePermissionTest & SSLConfigFilePermissionTest
tests
looked
very similar, so a common parent class was created for them:
AbstractFilePermissionTest
- What was called RmiRegistrySslTest.java I've renamed to
RmiRegistrySslTestApp.java. The java code to replace old
shell
script
RmiRegistrySslTest.sh is called RmiRegistrySslTest.java,
hence the
huge
diff.
- The new RmiRegistrySslTest.java has some lines similar
to the
AbstractFilePermissionTest.java, I nevertheless decided
to not
complicate the code further and leave it as is. Please
let me
know if
this is somehow not acceptable
- com/oracle/java/testlibrary/Utils.java that is added to
hotspot
repository is taken from this patch:
http://cr.openjdk.java.net/~ykantser/8023138/webrev.00/test/lib/testlibrary/jdk/testlibrary/Utils.java.sdiff.html
- These tests will need additional changes when test library
process
tools will support command line options inheritance
(http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-November/013235.html)
--
With best regards,
Taras Ledkov
Mail-To: taras.led...@oracle.com
skype: taras_ledkov
Phone: 7(812)3346-157
