Review request for 7195249: Some jtreg tests use hard coded ports
taras ledkov
taras.ledkov at oracle.com
Wed Feb 5 04:42:59 PST 2014
Hi,
So please take a look at the review against JDK9.
The reviewed patch had not been integrated into JDK8.
Port to JDK9 is identical. The difference: the ProcessTools.java has
been already patched by Jaroslav.
Webrev for jdk part:
http://cr.openjdk.java.net/~anazarov/7195249/jdk/webrev.03/
Webrev for hs part:
http://cr.openjdk.java.net/~anazarov/7195249/hs/webrev.03/
On 21.01.2014 13:45, Jaroslav Bachorik wrote:
> 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
>>>
>>
>
--
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