Review request for 7195249: Some jtreg tests use hard coded ports
Staffan Larsen
staffan.larsen at oracle.com
Wed Jan 8 05:46:01 PST 2014
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.
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.
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.
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.
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 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.
- Lines 57-58: Don’t swallow exceptions, add an ex.printStackTrace(). (Same thing for all other places where you call Integer.parseInt())
test/sun/management/jmxremote/bootstrap/Dummy.java:
This file is never used as far as I can see.
Thanks,
/Staffan
On 26 dec 2013, at 14:09, taras ledkov <taras.ledkov at 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.ledkov at oracle.com
> skype: taras_ledkov
> Phone: 7(812)3346-157
More information about the serviceability-dev
mailing list