Fwd: Re: Review request for 7195249: Some jtreg tests use hard coded ports
taras ledkov
taras.ledkov at oracle.com
Thu Dec 26 05:09:17 PST 2013
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