[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Fwd: Re: Review request for 7195249: Some jtreg tests use hard coded ports
From: taras ledkov <taras.ledkov () oracle ! com>
Date: 2013-12-26 13:09:17
Message-ID: 52BC2A7D.3070403 () oracle ! com
[Download RAW message or body]
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@oracle.com
skype: taras_ledkov
Phone: 7(812)3346-157
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic