Skip to content

8308986: Disable svc tests failing with virtual thread factory #14193

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

Closed
wants to merge 4 commits into from

Conversation

lmesnik
Copy link
Member

@lmesnik lmesnik commented May 27, 2023

Disable test
java/util/concurrent/locks/Lock/OOMEInAQS.java
Test provokes OOME and might catch it in an unexpected location. It looks like an issue similar to https://bugs.openjdk.org/browse/JDK-8298066. However, generally the OOME might be thrown mount/umount or in unparker thread. There are no plans to fix such tests to be able to pass with virtual threads now.

Also, disable
mTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t001/TestDescription.java
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t002/TestDescription.java
because of https://bugs.openjdk.org/browse/JDK-8308985


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8308986: Disable svc tests failing with virtual thread factory

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14193/head:pull/14193
$ git checkout pull/14193

Update a local copy of the PR:
$ git checkout pull/14193
$ git pull https://git.openjdk.org/jdk.git pull/14193/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14193

View PR using the GUI difftool:
$ git pr show -t 14193

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14193.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 27, 2023

👋 Welcome back lmesnik! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label May 27, 2023
@openjdk
Copy link

openjdk bot commented May 27, 2023

@lmesnik To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • build
  • client
  • compiler
  • core-libs
  • graal
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • ide-support
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah

@lmesnik
Copy link
Member Author

lmesnik commented May 27, 2023

/cc serviceability

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label May 27, 2023
@openjdk
Copy link

openjdk bot commented May 27, 2023

@lmesnik
The serviceability label was successfully added.

@mlbridge
Copy link

mlbridge bot commented May 27, 2023

Webrevs

@@ -72,6 +72,7 @@ java/lang/Thread/UncaughtExceptionsTest.java 0000000 generic-all
java/lang/Thread/virtual/GetStackTraceWhenRunnable.java 0000000 generic-all
java/lang/invoke/condy/CondyNestedResolutionTest.java 0000000 generic-all
java/lang/ref/OOMEInReferenceHandler.java 0000000 generic-all
java/util/concurrent/locks/Lock/OOMEInAQS.java 0000000 generic-all
Copy link
Contributor

@AlanBateman AlanBateman May 28, 2023

Choose a reason for hiding this comment

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

I read the comment a bit further up to see where it explains why this section uses "0000000" for the JBS issue. There's a few typos in the text, e.g. "with with" and "test migth". It might be useful to make that text a bit clearer as I'm sure there will be questions on why there aren't JBS issues for tests excluded from running this mode.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hotspot changes are good.

JDK side seems okay if this is how we are permanently excluding tests from running with virtual threads. Though as Alan states the comments need a little work.

Thanks.

@openjdk
Copy link

openjdk bot commented May 29, 2023

@lmesnik 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:

8308986: Disable svc tests failing with virtual thread factory

Reviewed-by: dholmes, dcubed, sspitsyn

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 30 new commits pushed to the master branch:

  • 1e6770f: 8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM
  • cb40db0: 8309134: Augment test/langtools/tools/javac/versions/Versions.java for JDK 21 language changes
  • de7fd1c: 8307944: ClassFileDumper should only load java.nio.file.Path if enabled
  • 7891de3: 8297885: misc sun/security/pkcs11 tests timed out
  • 323d6ce: 8308960: Decouple internal Version and OperatingSystem classes
  • 1b8e6bf: 8308987: Update java.lang.Class to use javadoc snippets
  • 04b0e78: 8307648: java/net/httpclient/ExpectContinueTest.java timed out
  • 6b90b05: 8297878: KEM: Implementation
  • 21af8ba: 8290499: new File(parent, "/") breaks normalization – creates File with slash at the end
  • 804f198: 8308992: New test TestHFA fails with zero
  • ... and 20 more: https://git.openjdk.org/jdk/compare/ca54f4e007ab0f13bec9aaf995d34c0ab3ba6452...master

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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 29, 2023
@@ -31,6 +31,8 @@ serviceability/dcmd/thread/PrintConcurrentLocksTest.java 8308033 generic-all
serviceability/dcmd/thread/PrintTest.java 8308033 generic-all
serviceability/dcmd/thread/ThreadDumpToFileTest.java 8308033 generic-all
serviceability/tmtools/jstack/DaemonThreadTest.java 8308033 generic-all
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t001/TestDescription.java 8308985 generic-all
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t002/TestDescription.java 8308985 generic-all
Copy link
Contributor

Choose a reason for hiding this comment

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

I've closed the 8308985 as a dup of:
8308978 regression with a deadlock involving FollowReferences
Should the 8308985 be replaced with the 8308978?
There is already a tested fix for 8308978.
Alex is going to post a PR today or tomorrow.
Do we still want to problem-list these two tests?

Copy link
Member

Choose a reason for hiding this comment

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

Please switch from 8308985 to 8308978. I've also changed this bug
into a sub-task of 8308978 and bumped the priorities of both bugs
to P2. Regressions start at P2 and ProblemListing match the priority
of their parent bug.

@dcubed-ojdk
Copy link
Member

Reducing noise in the CI is always good. Especially since it takes
ProblemListings and fixes a non-trivial amount of time to make it
to the upper tiers...

@@ -31,6 +31,8 @@ serviceability/dcmd/thread/PrintConcurrentLocksTest.java 8308033 generic-all
serviceability/dcmd/thread/PrintTest.java 8308033 generic-all
serviceability/dcmd/thread/ThreadDumpToFileTest.java 8308033 generic-all
serviceability/tmtools/jstack/DaemonThreadTest.java 8308033 generic-all
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t001/TestDescription.java 8308985 generic-all
vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t002/TestDescription.java 8308985 generic-all
Copy link
Member

Choose a reason for hiding this comment

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

Please switch from 8308985 to 8308978. I've also changed this bug
into a sub-task of 8308978 and bumped the priorities of both bugs
to P2. Regressions start at P2 and ProblemListing match the priority
of their parent bug.

@sspitsyn
Copy link
Contributor

Reducing noise in the CI is always good. Especially since it takes
Okay. We should not forget to unproblem-list the two test.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Choose a reason for hiding this comment

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

Looks good modulo my comment.
It is up to you to decide what bug number to use in the Hotspot problem list.
Also, there are more failing tests because of this issue. For instance:

  vmTestbase/nsk/jdi/VirtualMachine/instanceCounts/instancecounts001/instancecounts001.java
  vmTestbase/nsk/jdi/ReferenceType/instances/instances001/instances001.java
  vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java
  vmTestbase/nsk/jdi/stress/serial/heapwalking002/TestDescription.java

Thanks,
Serguei

@lmesnik
Copy link
Member Author

lmesnik commented May 30, 2023

I've excluded more tests, changed bugid and updated comments.

@dcubed-ojdk
Copy link
Member

I've been trying to figure out what java/util/concurrent/locks/Lock/OOMEInAQS.java
has to do with the deadlock bug, but I think I just got confused by using a single
bug ID to do the ProblemListings for more than one problem...

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up. Quite a few tests now for 8308978.

## There is no goal to run all test with virtual test thread factory.
## So any test migth be added as incompatible, the bug is not required.
## So any test might be added as incompatible, the bug is not required.
Copy link
Member

Choose a reason for hiding this comment

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

typo: s/bug/bug-ID/

@dcubed-ojdk
Copy link
Member

vmTestbase/nsk/jdi/VirtualMachine/instanceCounts/instancecounts002/TestDescription.java

has also shown up in the CI with same failure mode as the others...

@lmesnik
Copy link
Member Author

lmesnik commented May 31, 2023

/integrate

@openjdk
Copy link

openjdk bot commented May 31, 2023

Going to push as commit 327733c.
Since your change was applied there have been 30 commits pushed to the master branch:

  • 1e6770f: 8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM
  • cb40db0: 8309134: Augment test/langtools/tools/javac/versions/Versions.java for JDK 21 language changes
  • de7fd1c: 8307944: ClassFileDumper should only load java.nio.file.Path if enabled
  • 7891de3: 8297885: misc sun/security/pkcs11 tests timed out
  • 323d6ce: 8308960: Decouple internal Version and OperatingSystem classes
  • 1b8e6bf: 8308987: Update java.lang.Class to use javadoc snippets
  • 04b0e78: 8307648: java/net/httpclient/ExpectContinueTest.java timed out
  • 6b90b05: 8297878: KEM: Implementation
  • 21af8ba: 8290499: new File(parent, "/") breaks normalization – creates File with slash at the end
  • 804f198: 8308992: New test TestHFA fails with zero
  • ... and 20 more: https://git.openjdk.org/jdk/compare/ca54f4e007ab0f13bec9aaf995d34c0ab3ba6452...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 31, 2023
@openjdk openjdk bot closed this May 31, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 31, 2023
@openjdk
Copy link

openjdk bot commented May 31, 2023

@lmesnik Pushed as commit 327733c.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

5 participants