Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Dec 11, 2020

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258057: serviceability/attach/RemovingUnixDomainSocketTest.java doesn't ignore VM warnings

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1749/head:pull/1749
$ git checkout pull/1749

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 11, 2020

👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@pchilano The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-runtime
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 11, 2020

/label remove core-libs

Loading

@openjdk openjdk bot removed the core-libs label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@pchilano
The core-libs label was successfully removed.

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 11, 2020

/label remove hotspot-runtime

Loading

@openjdk openjdk bot removed the hotspot-runtime label Dec 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 11, 2020

@pchilano
The hotspot-runtime label was successfully removed.

Loading

@pchilano pchilano marked this pull request as ready for review Dec 11, 2020
@openjdk openjdk bot added the rfr label Dec 11, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 11, 2020

Webrevs

Loading

@alexmenkov
Copy link

@alexmenkov alexmenkov commented Dec 11, 2020

Pumpers created by ProcessTools.startProcess() log the process stdout/stderr.
It's quite useful for failure analysis.

Loading

Copy link
Contributor

@plummercj plummercj left a comment

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?

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Dec 11, 2020

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.

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 11, 2020

Hi Alex,

Pumpers created by ProcessTools.startProcess() log the process stdout/stderr.
It's quite useful for failure analysis.
But we are still printing the stdout and stderr of the jcmd with the log statement in runJCmd().

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 11, 2020

Hi Chris,

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 also did a search and the other serviceability tests I found using stderrShouldBeEmtpyIgnoreWarnings() execute the process with ProcessBuilder::start().

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?

Looking at ProcessTools.startProcess(), it just calls ProcessBuilder::start() and then executes all the other StreamPumper logic.

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?

Right. Otherwise we have a race and the output of the launched process will be split between the two StreamPumpers in an unpredictable way.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 14, 2020

@pchilano This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: cjplummer, amenkov, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 70 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Dec 14, 2020
@@ -66,7 +67,7 @@ private static void runJCmd(long pid) throws InterruptedException, IOException {
"jcmd exitValue = " + out.getExitValue());

out.shouldHaveExitValue(0);
out.stderrShouldBeEmptyIgnoreVMWarnings();
out.stderrShouldBeEmptyIgnoreDeprecatedWarnings();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at the fix one more time.

I suppose this new method is not needed as a the message about deprecation is VM warning and should be handled by stderrShouldBeEmptyIgnoreVMWarnings.
Also this new method will fails if jcmd prints some other VM warning

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 15, 2020

Mailing list message from David Holmes on serviceability-dev:

On 15/12/2020 10:01 am, Alex Menkov wrote:

On Fri, 11 Dec 2020 17:25:33 GMT, Patricio Chilano Mateo <pchilanomate 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

Changes requested by amenkov (Reviewer).

test/hotspot/jtreg/serviceability/attach/RemovingUnixDomainSocketTest.java line 70:

68:
69: out.shouldHaveExitValue(0);
70: out.stderrShouldBeEmptyIgnoreDeprecatedWarnings();

Looked at the fix one more time.

I suppose this new method is not needed as a the message about deprecation is VM warning and should be handled by stderrShouldBeEmptyIgnoreVMWarnings.
Also this new method will fails if jcmd prints some other VM warning

I think that is the point of the new method. We know deprecation
warnings are harmless and can be ignored. But other warnings may
indicate an issue that needs investigating.

Cheers,
David

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 15, 2020

Mailing list message from David Holmes on serviceability-dev:

On 15/12/2020 10:01 am, Alex Menkov wrote:

On Fri, 11 Dec 2020 17:25:33 GMT, Patricio Chilano Mateo 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

Changes requested by amenkov (Reviewer).
test/hotspot/jtreg/serviceability/attach/RemovingUnixDomainSocketTest.java line 70:

68:
69: out.shouldHaveExitValue(0);
70: out.stderrShouldBeEmptyIgnoreDeprecatedWarnings();

Looked at the fix one more time.
I suppose this new method is not needed as a the message about deprecation is VM warning and should be handled by stderrShouldBeEmptyIgnoreVMWarnings.
Also this new method will fails if jcmd prints some other VM warning

I think that is the point of the new method. We know deprecation
warnings are harmless and can be ignored. But other warnings may
indicate an issue that needs investigating.

Cheers,
David
Right, as David pointed out the purpose is to only ignore deprecation warnings.

Patricio

Loading

@@ -40,6 +40,8 @@

private static final String jvmwarningmsg = ".* VM warning:.*";

private static final String deprecatedmsg = ".* deprecated.*";
Copy link
Member

@dholmes-ora dholmes-ora Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be too generic, perhaps combine it with "VM warning" so we don't get accidental hits on other deprecation output eg from javac?
Also I'm not sure if only deprecation warnings should be handled but I guess we can adjust if that need arises.

Loading

Copy link
Contributor Author

@pchilano pchilano Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I added "VM warning" to the search.

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Thanks!

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 17, 2020

Thanks @alexmenkov, @plummercj and @dholmes-ora for the reviews!

Loading

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Dec 17, 2020

/integrate

Loading

@openjdk openjdk bot closed this Dec 17, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 17, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2020

@pchilano Since your change was applied there have been 74 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 7b05439.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants