Review request for 7195249: Some jtreg tests use hard coded ports
taras ledkov
taras.ledkov at oracle.com
Mon Jan 20 07:07:34 PST 2014
Hi Staffan,
I fixed the tests according with your comments.
Are you OK?
On 15.01.2014 19:15, taras ledkov wrote:
> 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