Review request for 7195249: Some jtreg tests use hard coded ports

taras ledkov taras.ledkov at oracle.com
Wed Jan 15 07:15:38 PST 2014 

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

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