Review request for 7195249: Some jtreg tests use hard coded ports
taras ledkov
taras.ledkov at oracle.com
Tue Jan 21 01:30:37 PST 2014
Hi Jaroslav,
Could you please review the last changes?
Are you OK?
On 20.01.2014 19:21, Staffan Larsen wrote:
> 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
>
--
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