8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out #13969

sspitsyn · 2023-05-13T07:28:38Z

This is newly integrated test times out because it has a race in in the Test #A.1 and #A.2.
The main root cause is a print statement which can case target virtual thread to unpark and unmount.
This causes that the StopThreads unexpectedly fails with the JVMTI_ERROR_OPAQUE_FRAME error code.
The target thread can be in some other unexpected states if JVMTI StopThread
is called before the target thread method A() reached the synchronized statement.

The fix is to replace the ensureStarted() with the ensureAtPointA().
The fix also includes some simplifications related to clearing the target thread interrupt status.

Testing:
Hundreds of mach5 runs of serviceability/jvmti/vthread tests which include the fixed StopThreadTest.
TBD: To run mach5 tiers1-3.

The test does not fail with this fix anymore.

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-8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 58080db4
Leonid Mesnik (@lmesnik - Reviewer) ⚠️ Review applies to 58080db4

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13969

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

Using diff file

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

Webrev

Link to Webrev Comment

…ava timed out

bridgekeeper · 2023-05-13T07:29:46Z

👋 Welcome back sspitsyn! 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 · 2023-05-13T07:31:47Z

@sspitsyn The following label will be automatically applied to this pull request:

serviceability

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

mlbridge · 2023-05-13T07:35:58Z

Webrevs

plummercj · 2023-05-15T21:41:11Z

test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java

            }
            if (!seenExceptionFromA) {
                StopThreadTest.setFailed("TestTask.run: expected AssertionError from method A()");
            }
+            Thread.interrupted();


I'm not sure why this was move down this far. It seems you would want to do this before the StopThreadTest.setFailed() call just to be extra safe, so that means move it before the if statement.

Good suggestion. Fixed.

plummercj · 2023-05-15T21:44:37Z

test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java

@@ -248,6 +240,7 @@ public void run() {
        //  - when suspended: JVMTI_ERROR_NONE is expected
        static void A() {
            log("TestTask.A: started");
+            atPointA = true;


What happens if we are at the point where this flag has been set true, but we have not yet executed the monitorenter? Will the test pass if the StopThread is done a bit early here?

Yes, the test will pass as the target virtual thread should still be in a good expected state.
#A.1: The StopThread should still return the JVMTI_ERROR_THREAD_NOT_SUSPENDED error code.
#A.2: The StopThread should return the JVMTI_ERROR_NONE as the target virtual thread has been mounted.

…ines

openjdk · 2023-05-16T01:35:27Z

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

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out

Reviewed-by: cjplummer, lmesnik

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

d22bcc8: 8307403: java/util/zip/DeInflate.java timed out
2210e06: 8306638: Open source some AWT tests related to datatransfer and Toolkit
05b51c7: 8307567: Avoid relocating global roots to metaspaceObjs in CDS dump
3168372: 8306597: Improve string formatting in EquivMapsGenerator.java
7b0b9b5: 8308049: Refactor nested class declarations in FdLibm.java
cc80ada: 8306593: Fix nsk/jdi/stop/stop001 for virtual threads and remove from problem list
43c8c65: 8307555: Reduce memory reads in x86 MD5 intrinsic
01892f9: 8304913: Use OperatingSystem, Architecture, and Version in jlink
0f7b1c5: 8299340: CreateProcessW lpCommandLine must be mutable
57e7a3f: 8307959: Remove explicit type casts from SerializeClosure::do_xxx() calls
... and 11 more: https://git.openjdk.org/jdk/compare/e54051ae9e1cdaa51a2d569227996a96b79bfa0d...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.

lmesnik

There is one nit related to code style.

lmesnik · 2023-05-16T01:57:39Z

test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java

@@ -104,7 +104,7 @@ public static void run() {
            } else {
                testTaskThread = Thread.ofPlatform().name("TestTaskThread").start(testTask);
            }
-            testTask.ensureStarted();
+            testTask.ensureAtPointA();


Java Code Conventions recommends to use className and not variables while calling static methods. Same for 'ensureFinished()'.

Thank you. Fixed now.

Chris and Leonid, thank you for review!

sspitsyn · 2023-05-16T07:18:45Z

/integrate

openjdk · 2023-05-16T07:20:13Z

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

d22bcc8: 8307403: java/util/zip/DeInflate.java timed out
2210e06: 8306638: Open source some AWT tests related to datatransfer and Toolkit
05b51c7: 8307567: Avoid relocating global roots to metaspaceObjs in CDS dump
3168372: 8306597: Improve string formatting in EquivMapsGenerator.java
7b0b9b5: 8308049: Refactor nested class declarations in FdLibm.java
cc80ada: 8306593: Fix nsk/jdi/stop/stop001 for virtual threads and remove from problem list
43c8c65: 8307555: Reduce memory reads in x86 MD5 intrinsic
01892f9: 8304913: Use OperatingSystem, Architecture, and Version in jlink
0f7b1c5: 8299340: CreateProcessW lpCommandLine must be mutable
57e7a3f: 8307959: Remove explicit type casts from SerializeClosure::do_xxx() calls
... and 11 more: https://git.openjdk.org/jdk/compare/e54051ae9e1cdaa51a2d569227996a96b79bfa0d...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-16T07:20:28Z

@sspitsyn Pushed as commit c2ef302.

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

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.j…

bc3d1b4

…ava timed out

openjdk bot added the rfr Pull request is ready for review label May 13, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label May 13, 2023

plummercj reviewed May 15, 2023

View reviewed changes

moved Thread.interrupted() in StopThreadTest.java above a couple of l…

58080db

…ines

plummercj approved these changes May 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 16, 2023

lmesnik approved these changes May 16, 2023

View reviewed changes

use class name for static calls

d960050

openjdk bot added the integrated Pull request has been integrated label May 16, 2023

openjdk bot closed this May 16, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 16, 2023

sspitsyn deleted the br33 branch May 16, 2023 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out #13969

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out #13969

sspitsyn commented May 13, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 13, 2023

openjdk bot commented May 13, 2023

mlbridge bot commented May 13, 2023 •

edited

Loading

plummercj May 15, 2023

sspitsyn May 16, 2023

plummercj May 15, 2023

sspitsyn May 16, 2023 •

edited

Loading

openjdk bot commented May 16, 2023 •

edited

Loading

lmesnik left a comment

lmesnik May 16, 2023

sspitsyn May 16, 2023

sspitsyn May 16, 2023

sspitsyn commented May 16, 2023

openjdk bot commented May 16, 2023

openjdk bot commented May 16, 2023

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out #13969

8307968: serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java timed out #13969

Conversation

sspitsyn commented May 13, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 13, 2023

openjdk bot commented May 13, 2023

mlbridge bot commented May 13, 2023 • edited Loading

Webrevs

plummercj May 15, 2023

Choose a reason for hiding this comment

sspitsyn May 16, 2023

Choose a reason for hiding this comment

plummercj May 15, 2023

Choose a reason for hiding this comment

sspitsyn May 16, 2023 • edited Loading

Choose a reason for hiding this comment

openjdk bot commented May 16, 2023 • edited Loading

lmesnik left a comment

Choose a reason for hiding this comment

lmesnik May 16, 2023

Choose a reason for hiding this comment

sspitsyn May 16, 2023

Choose a reason for hiding this comment

sspitsyn May 16, 2023

Choose a reason for hiding this comment

sspitsyn commented May 16, 2023

openjdk bot commented May 16, 2023

openjdk bot commented May 16, 2023

sspitsyn commented May 13, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 13, 2023 •

edited

Loading

sspitsyn May 16, 2023 •

edited

Loading

openjdk bot commented May 16, 2023 •

edited

Loading