Review request for 7195249: Some jtreg tests use hard coded ports
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Tue Jan 21 01:45:31 PST 2014
Hi Taras,
On 21.1.2014 10:30, taras ledkov wrote:
> Hi Jaroslav,
>
> Could you please review the last changes?
> Are you OK?
Yes, the change looks ok. But I think we will need to get back to this
problem eventually and implement a central port dispatcher if we want to
be 100% sure the port conflicts wouldn't occur. But your changes reduce
the chance significantly.
Thanks for taking care of this.
-JB-
>
> 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
>>
>
More information about the serviceability-dev
mailing list