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. 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. 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. 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. 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 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. - Lines 57-58: Don’t swallow exceptions, add an ex.printStackTrace(). (Same thing for all other places where you call Integer.parseInt()) test/sun/management/jmxremote/bootstrap/Dummy.java: This file is never used as far as I can see. Thanks, /Staffan On 26 dec 2013, at 14:09, taras ledkov <taras.led...@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.led...@oracle.com > skype: taras_ledkov > Phone: 7(812)3346-157
