On Fri, 11 Dec 2020 20:46:42 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Hi, >> >> Please review the following small fix for test >> RemovingUnixDomainSocketTest.java. As explained in the bug comments, the >> issue is due to having two different StreamPumper objects consuming from the >> same stderr, one created by ProcessTools.startProcess() and another by >> OutputAnalyzer(). In the failing cases the StreamPumper processing thread >> created in ProcessTools.startProcess() consumes the first part of the >> deprecation message while the one created in OutputAnalyzer consumes the >> rest. Since out.getStderr() is not empty and does not contain the string "VM >> warning:", the test fails. >> >> I simply replaced the ProcessTools.startProcess() call by a call to start() >> on the ProcessBuilder object, which doesn't use StreamPumper. I added >> stderrShouldBeEmptyIgnoreDeprecatedWarnings(), since as mentioned in 8248162 >> we might not want to hide all warning messages. >> >> Without the patch I can consistently reproduce the issue. With the patch the >> test always passes. >> >> Thanks, >> Patricio > > From what I can tell (after a bit of possibly inaccurate grepping) this is > the only test that uses `PrcoessTools.startProcess()` in combination with > `out.stderrShouldBeEmtpyIgnoreWarnings()`, so I assume this issue of split > stderr output is unique to this test. However, it seems like that could > easily be stumbled into again, and I pity anyone who has to debug this again > (and commend you and getting to the root of the issue). > > I have to admit I don't understanding all the ramifications of moving from > `ProcessTools.startProcess()` to just calling `pb.Start()`. Clearly > `startProcess()` has some value add. Does not using it affect the test in any > negative way? > > It's also not clear to me what the guidelines are for avoiding this issue in > the future. Is it that `startProcess()` + `OutputAnalyzer` on the same > process is forbidden, or at least forbidden if the `OutputAnalyzer` is used > for anything more than checking the exit value? > Pumpers created by ProcessTools.startProcess() log the process stdout/stderr. > It's quite useful for failure analysis. This was the type of thing I was hinting at when I asked if there were any possible negative side affects to the change. Sorry I didn't see your post before asking my question. ------------- PR: https://git.openjdk.java.net/jdk/pull/1749
