Review request for 7195249: Some jtreg tests use hard coded ports
Staffan Larsen
staffan.larsen at oracle.com
Mon Jan 20 07:21:12 PST 2014
Sorry for not replying earlier. Yes, I’m ok with these changes.
Thanks,
/Staffan
On 20 jan 2014, at 16:07, taras ledkov <taras.ledkov at oracle.com> wrote:
> 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