Hi Daniil, On Wed, 2019-07-17 at 18:26 -0700, Daniil Titov wrote: > Hi Chris, > > Yes, I added output.reportDiagnosticSummary() in webrev-01, but removed it > In webrev-02, and later restored it in webrev-03. > > When the test runs of retries output.getExitValue() is set to 1 > (COMMUNICATION_ERROR_EXIT_VAL) > (the "exitValue =1 " in the previous email). > > Please review a new version of the fix that has an explicit error message > printed > if the test runs out of retries (line 168). > > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.04/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
This looks reasonable to me. Thanks for fixing it! Thanks, Severin > Thanks! > --Daniil > > On 7/17/19, 5:33 PM, "Chris Plummer" <chris.plum...@oracle.com> > wrote: > > Hi Daniil, > > I'm confused now. I mentioned output.reportDiagnosticSummary() > because I > thought I saw it in your webrev-02. Now I don't. Maybe I had > glanced > back at webrev-01 and saw it there. One issue with exceptions in > the > output that are not considered errors is that if later there is > an > error, mdash wills show the exception as one of (possible > multiple) > reasons for the failure, so it's good to avoid if possible. Looks > like > what you have now is ok. > > I have a question about what happens if you run out of retries. > What is > output.getExitValue() set to? Also, I think you should print an > an > explicit error message indicating you've run out of retries. > > thanks, > > Chris > > On 7/17/19 4:36 PM, Daniil Titov wrote: > > Hi Chris, > > > > output.reportDiagnosticSummary() prints the output from the > process (both stdout and stderr) and the exit value to the test's > stderr. > > In case if the port is already in use it prints the following: > > > > stdout: []; > > stderr: [Error: JMX connector server communication error: > service:jmx:rmi://127.0.0.1:9101 > > jdk.internal.agent.AgentConfigurationError: > java.rmi.server.ExportException: Port already in use: 9101; nested > exception is: > > java.net.BindException: Address already in use > > at > jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.expo > rtMBeanServer(ConnectorBootstrap.java:820) > > at > jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.star > tRemoteConnectorServer(ConnectorBootstrap.java:479) > > at > jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:4 > 47) > > at > jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:5 > 99) > > Caused by: java.rmi.server.ExportException: Port already in > use: 9101; nested exception is: > > < I skipped the rest> > > ] > > exitValue = 1 > > > > It makes sense to have it called if the test fails, otherwise > this information would be missed in the test output. > > Please review the new version of the fix that has the call to > output.reportDiagnosticSummary() restored. > > > > Thanks! > > > > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.03/ > > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303 > > > > -Daniil > > > > On 7/17/19, 3:58 PM, "Chris Plummer" <chris.plum...@oracle.com > > wrote: > > > > What does output.reportDiagnosticSummary() print out then > the port is > > already in use, and have you made this happen with your > fixes in place? > > > > Chris > > > > On 7/17/19 3:20 PM, Daniil Titov wrote: > > > Hi Chris and Alex, > > > > > > Please review a new version of the fix that moves the > diagnostic output for the test failure to run() > > > method after the number of retry attempts is exceeded. > It also includes other corrections that > > > you suggested. > > > > > > Thanks! > > > > > > Webrev: > http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/ > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303 > > > > > > --Best regards, > > > Daniil > > > > > > > > > On 7/17/19, 1:47 PM, "Chris Plummer" < > chris.plum...@oracle.com> wrote: > > > > > > Hi Daniil, > > > > > > I think you can remove "Ok" from the following > message: > > > > > > 237 System.out.println("DEBUG: OK. Spawned java > process terminated > > > with expected exit code of " > > > 238 + > STOP_PROCESS_EXIT_VAL); > > > > > > It's somewhat misleading since the test can still > fail. I think the > > > following is also misleading: > > > > > > 249 if (testFailed) { > > > 250 output.reportDiagnosticSummar > y(); > > > 251 if > (output.getStderr().contains("Port already in > > > use")) { > > > 252 // Need to retry > > > 253 return true; > > > 254 } > > > 255 } > > > > > > The test can still pass after this happens, right? > And I think there are > > > other "Test FAILURE" error message that can appear > just before this. > > > Perhaps the code above should add a println that > indicates that there > > > will be a retry attempt since the failure was due > to the port being in use. > > > > > > Otherwise looks good. > > > > > > thanks, > > > > > > Chris > > > > > > On 7/17/19 1:30 PM, Alex Menkov wrote: > > > > Hi Daniil, > > > > > > > > The fix looks good in general. > > > > Couple cosmetic notes (no new webrev required): > > > > > > > > 162 needRetry = runTest(); > > > > 163 } > > > > 164 while (needRetry && (attempts++ > < MAX_RETRY_ATTEMTS)); > > > > Please move "while" to the prev line: > > > > 163 } while (needRetry && > (attempts++ < MAX_RETRY_ATTEMTS)); > > > > > > > > > > > > 242 System.out.println("Test > FAILURE on" + name + > > > > " reason: The expected line \"" + READY_MSG > > > > 243 + "\" is not > present in the process > > > > output"); > > > > Please add space: "Test FAILURE on " + name > > > > > > > > --alex > > > > > > > > On 07/17/2019 12:46, Daniil Titov wrote: > > > >> Hi Chris, > > > >> > > > >>> It's a little unclear to me why you moved > from ProcessThread to > > > >>> TestProcessThread + Process. An explanation > of that would make it > > > >>> easier > > > >>> to understand many of the changes. > > > >> > > > >> There are two reasons for that: > > > >> 1) For every network interface the test starts > a separate thread > > > >> that runs the test for this interface. We want > that if the test fails > > > >> due to the bind error the thread not to > exit but try to repeat > > > >> the test several times. > jdk.test.lib.thread.ProcessThread doesn't > > > >> allow this. > > > >> 2) To filter out the cases when the test fails > due to the bind error > > > >> we need to parse the process output. > jdk.test.lib.thread.ProcessThread > > > >> registers its own pumps for stdin and > sdtout (by calling > > > >> ProcessTools.startProcess()) that consume all > output of the process > > > >> and prevent > > > >> the output analyzer from collecting this > output. > > > >> Thanks! > > > >> --Daniil > > > >> > > > >> On 7/17/19, 12:11 PM, "Chris Plummer" < > chris.plum...@oracle.com> wrote: > > > >> > > > >> Hi Daniil, > > > >> It's a little unclear to me why you > moved from > > > >> ProcessThread to > > > >> TestProcessThread + Process. An explanation > of that would make > > > >> it easier > > > >> to understand many of the changes. > > > >> thanks, > > > >> Chris > > > >> On 7/11/19 10:16 AM, Daniil Titov > wrote: > > > >> > Please review the change that fixes an > intermittent failure of > > > >> > sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > > > >> > test due to ports collision. The tests > finds all network > > > >> interfaces and for every interface starts a > separate process that tests > > > >> > the connection to JMX agent server for a > specific > > > >> ports/interface combination. > > > >> > > > > >> > The test was changed to retry in case of > the failure. If the > > > >> subprocess fails to bind and the number > > > >> > of retry attempts doesn't exceed the > limit a new pair of > > > >> random ports is selected and the test is run > again. > > > >> > > > > >> > Webrev: > http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/ > > > >> > Bug: > https://bugs.openjdk.java.net/browse/JDK-8221303 > > > >> > > > > >> > Thanks! > > > >> > --Daniil > > > >> > > > > >> > > > > >> > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > >
