8232839: JDI AfterThreadDeathTest.java failed due to "FAILED: Did not get expected IllegalThreadStateException on a StepRequest.enable()"

[plummercj]

The test waits for a ThreadDeathEvent for "main". Once that arrives, it then waits for the next ThreadStartEvent (for any thread). Once it arrives, the test tries to create and enable a StepRequest on the "main" thread. Since "main" is supposedly dead, the expectation is an IllegalThreadStateException. However, it turns out that sometimes the enabling can in fact succeed.

Just because a ThreadDeathEvent has been received for a thread does not mean you can no longer do things with the thread like suspend it or enable a StepEvent. There is a short delay in the debug agent after sending the ThreadDeathEvent before it stops tracking the thread. The thread can still be acted upon until then. The JDWP and JDI specs seem to support doing this:

Notification of a completed thread in the target VM. The notification is generated by the dying thread before it terminates. Because of this timing, it is possible for {@link VirtualMachine#allThreads} to return this thread after this event is received.

Note that this event gives no information about the lifetime of the thread object. It may or may not be collected soon depending on what references exist in the target VM.

What this means is that when the test receives some arbitrary ThreadStartEvent after the "main" ThreadDeathEvent has been received, the test may in fact still be able to enable a StepRequest on the "main" thread, causing the test to fail. What seems to trigger the failure is receiving an unexpected spurious ThreadStartEvent such as from the Common-Clean thread or a carrier thread, although even then it only fails some of the time. In fact if I modify the test to enable the StepRequest when it receives the ThreadDeathEvent for "main", it still almost always passes, but will fail more frequently than it normally does.

It seems if the test always waits for the ThreadStartEvent for "DestroyJavaVM", then the "main" thread is truly gone by then and the test always passes, so this is how I've chosen to fix the issue.

Tested with tier1, tier2 svc tests, and tier5 svc tests.

---

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-8232839: JDI AfterThreadDeathTest.java failed due to "FAILED: Did not get expected IllegalThreadStateException on a StepRequest.enable()" (Bug - P4)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)
• Kevin Walls (@kevinjwalls - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14372

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back cjplummer! 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]

@plummercj 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]

Webrevs

• 00: Full (aab0b115)

[sspitsyn]

It looks good.
Thanks,
Serguei

[openjdk]

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

8232839: JDI AfterThreadDeathTest.java failed due to "FAILED: Did not get expected IllegalThreadStateException on a StepRequest.enable()"

Reviewed-by: sspitsyn, kevinw

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

• 34f0a6e: 8305593: Add @SPEC tags in java.desktop
• 0be3905: 8309595: Allow javadoc to process unnamed classes
• b2a5271: 8309196: Remove Thread.countStackFrames
• bb96682: 8309498: [JVMCI] race in CallSiteTargetValue recording
• 5a706fb: 8306584: Start of release updates for JDK 22
• bb377b2: 8306841: Generational ZGC: NMT reports Java heap size larger than max heap size
• ac3ce2b: 8304425: ClassHierarchyResolver from Reflection
• 79a4ac7: 8309235: Unnamed Variables (_) can't be used in JShell
• 9d64a9d: 8306431: File.listRoots method description should be re-examined
• 5af9d2a: 8309602: update JVMTI history table for jdk 21
• ... and 10 more: https://git.openjdk.org/jdk/compare/4ffc8cc2169c6b34ce5d9cf0ce98125520c4495a...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.

[kevinjwalls]

I think it makes sense!

[plummercj]

thanks for the reviews Kevin and Serguei!

/integrate

[openjdk]

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

• 679a6d8: 8309303: jdk/internal/misc/VM/RuntimeArguments test ignores jdk/internal/vm/options
• 6cd370e: 8305645: System Tray icons get corrupted when Windows primary monitor changes
• a48bcf3: 8308336: Test java/net/HttpURLConnection/HttpURLConnectionExpectContinueTest.java failed: java.net.BindException: Address already in use
• c052756: 8282797: CompileCommand parsing errors should exit VM
• dc842e8: 8309665: Simplify Arrays.copyOf/-Range methods
• 3070856: 8309702: Exclude java/lang/ScopedValue/StressStackOverflow.java from JTREG_TEST_THREAD_FACTORY=Virtual runs
• 97df6cf: 8309545: Thread.interrupted from virtual thread needlessly resets interrupt status
• f91e9ba: 8309612: [REDO] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING
• 0a697e7: 8307620: [IR Framework] Readme mentions JTREG_WHITE_LIST_FLAGS instead of JTREG_WHITELIST_FLAGS
• 6f492e8: 8309686: inconsistent URL for https://www.unicode.org/reports/tr35
• ... and 21 more: https://git.openjdk.org/jdk/compare/4ffc8cc2169c6b34ce5d9cf0ce98125520c4495a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@plummercj Pushed as commit 84184f9.

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