Sorry for not replying earlier. Yes, I’m ok with these changes. Thanks, /Staffan
On 20 jan 2014, at 16:07, taras ledkov <[email protected]> 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 <[email protected]> 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: [email protected] >>>> skype: taras_ledkov >>>> Phone: 7(812)3346-157 >>> >> > > -- > With best regards, > Taras Ledkov > Mail-To: [email protected] > skype: taras_ledkov > Phone: 7(812)3346-157
