On Tue, 23 Feb 2021 03:08:16 GMT, Ioi Lam <[email protected]> wrote: >> test/lib/jdk/test/lib/apps/LingeredApp.java line 419: >> >>> 417: theApp.waitAppReady(); >>> 418: } catch (Exception ex) { >>> 419: theApp.finishApp(); >> >> I'm worried about the output appearing twice in some tests: >> >> try { >> ... >> theApp = new LingeredAppWithLargeArray(); >> LingeredApp.startAppExactJvmOpts(theApp, vmArgs); >> attachAndDump(heapDumpFileName, theApp.getPid()); >> } finally { >> LingeredApp.stopApp(theApp); >> heapDumpFile.delete(); >> } >> >> So we always call `stopApp()`: >> >> public void stopApp() throws IOException { >> ... >> if (appProcess != null) { >> ... >> finishApp(); >> ... >> } >> } >> >> >> Which means we always call `finishApp()`, which means we always print the >> output. With your changes won't we see the output twice when there is an >> exception? >> >> In the CR it looks like `LingeredApp.startAppExactJvmOpts()` is being call >> from `JCmdTest.java`. I don't see this test. Is it under development? Does >> it need a `stopApp()` call? > > It seems error prone to have to call finishApp() manually in order to see the > error log. Since LingeredApp.startApp calls finishApp() on exceptions, > shouldn't startAppExactJvmOpts do the same thing?
Although you have a point, you've also pointed out another problem with this fix. I think users of `startApp()` are already going to see the output twice because of the `finishApp()` call present there. By adding yet another `finishApp()` call to `startAppExactJvmOpts()`, now they will see it 3 times. If you want to "move" the `finishApp()` call from `startApp()` to `startAppExactJvmOpts()`, then at least that will maintain the status quo with existing `startApp()` users, but we still have an issue with the output appearing twice, even before this change, and with this change it is now more common as `startAppExactJvmOpts()` will also start seeing it. Maybe `finishApp()` should maintain an `alreadyCalled` flag so it does nothing on subsequent calls. ------------- PR: https://git.openjdk.java.net/jdk/pull/2679
