Hi Daniil,
It looks Okay in general.
But I've got a question.
What is going to happen if all ports that we try are already in use?
Does the test report this situation?
Thanks,
Serguei
On 3/17/20 11:40, Daniil Titov wrote:
It looks Okay in general.
But I've got a question.
329 while (jstatdThread == null) { 330 if (!useDefaultPort) { 331 port = String.valueOf(Utils.getFreePort()); 332 } 333 334 if (!useDefaultRmiPort) { 335 rmiPort = String.valueOf(Utils.getFreePort()); 336 } 337 338 if (withExternalRegistry) { 339 Registry registry = startRegistry(); 340 if (registry == null) { 341 // The port is already in use. Cancel and try with a new one. 342 continue; 343 } 344 } 345 346 jstatdThread = tryToSetupJstatdProcess(); 347 }
What is going to happen if all ports that we try are already in use?
Does the test report this situation?
Thanks,
Serguei
On 3/17/20 11:40, Daniil Titov wrote:
Hi Alex,Please review a new version of the fix that removes the old version of the code that tried to handle the "port in use" case. Testing: Mach5 tests for sun/tools/jstatd/ successfully passed 100 times. Tier1-tier3 tests successfully passed. [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.02 [2] https://bugs.openjdk.java.net/browse/JDK-8240711 Thanks, Daniil On 3/16/20, 5:38 PM, "Daniil Titov" <daniil.x.ti...@oracle.com> wrote: Hi Alex, Yes, I did test the change by modifying the test to use the RMI port that is already in use ( the stack trace in the original email was exact from this changed test) and then ensured that with the fix the such issue is properly handled. I will send a new version of the webrev that removes the old version of the code that tried to handle the "port in use" case. Thanks! Best regards, Daniil On 3/16/20, 4:47 PM, "Alex Menkov" <alexey.men...@oracle.com> wrote: I don't agree. The code handles exact the same "port in use" case for the same tool. So it either works or doesn't. And have 2 code blocks which suppose to do the same makes the code messy. BTW did you tested the change (I mean craft the test to get "port in use" error)? --alex On 03/16/2020 16:17, Daniil Titov wrote: > Resending with the corrected subject ... > > Hi Alex, > > Yes, you are right, class JstatdTest has the code that is supposed to handle the "port in use" > case but at least for this specific test (sun/tools/jstatd/TestJstatdPort.java) it doesn't work. > > Since there are multiple tests in sun/tools/jstatd/* folder that use this class and different ports > might be subject to the "port in use" error and taking into account that it's hard to reproduce such case > I found it safer to leave the original code and just augment it with what was missing for this specific > case rather than completely replacing it. > > Best regards, > Daniil > > On 3/16/20, 4:02 PM, "Alex Menkov" <alexey.men...@oracle.com> wrote: > > Hi Daniil, > > Looks like the test is supposed to handle "port in use" issue (see lines > 103-114). > I suppose in case "port in use" jstatd exits, but > ProcessTools.startProcess() continue to wait for "jstatd started" message. > > --alex > > On 03/16/2020 12:00, Daniil Titov wrote: > > Please review the change [1] that fixes the intermittent failure of the test. > > > > The problem here is that if the RMI port is in use than the test keep waiting for "jstatd started (bound to " to appear in the process output and in this case > > It doesn't happen. > > > > at java.util.concurrent.CountDownLatch.await(java.base@15-internal/CountDownLatch.java:232) > > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:205) > > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:133) > > at jdk.test.lib.process.ProcessTools.startProcess(ProcessTools.java:254) > > at jdk.test.lib.thread.ProcessThread$ProcessRunnable.xrun(ProcessThread.java:153) > > at jdk.test.lib.thread.XRun.run(XRun.java:40) > > at java.lang.Thread.run(java.base@15-internal/Thread.java:832) > > at jdk.test.lib.thread.TestThread.run(TestThread.java:123) > > > > Testing: Mach5 tests for sun/tools/jstatd/ successfully passed. Tier1-tier3 tests are still in progress. > > > > [1] http://cr.openjdk.java.net/~dtitov/8240711/webrev.01/ > > [2] https://bugs.openjdk.java.net/browse/JDK-8240711 > > > > > > Thank you, > > Daniil > > > > > > > > >