RFR: 8258057: serviceability/attach/RemovingUnixDomainSocketTest.java doesn't ignore VM warnings

[Chris Plummer]

On Fri, 11 Dec 2020 20:46:42 GMT, Chris Plummer <cjplummer at 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