8361602: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java deadlocks on exception

[reinrich]

This pr moves the notify() call to the finally block to avoid the deadlock in join() if verifyHeapDump(dumpFile) throws an exception.

Testing was done with fastdebug and release builds on the main platforms and also on Linux/PPC64le and AIX.

---

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-8361602: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java deadlocks on exception (Bug - P4)

Reviewers

• Christoph Langer (@RealCLanger - Reviewer) Review applies to e802fe62
• David Holmes (@dholmes-ora - Reviewer) Review applies to 13cb9ba3
• Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26213

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8361602: [TESTBUG] serviceability/HeapDump/UnmountedVThreadNativeMethodAtTop.java deadlocks on exception

Reviewed-by: cjplummer, clanger, 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 41 new commits pushed to the master branch:

• 59bec29: 8358580: Rethink how classes are kept alive in training data
• a86dd56: 8360936: Test compiler/onSpinWait/TestOnSpinWaitAArch64.java fails after JDK-8359435
• 76442f3: 8361494: [IR Framework] Escape too much in replacement of placeholder
• ... and 38 more: https://git.openjdk.org/jdk/compare/e0245682c8d5a0daae055045c81248c12fb23c09...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]

@reinrich 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

• 03: Full - Incremental (c2ae5b24)
• 02: Full - Incremental (13cb9ba3)
• 01: Full - Incremental (a4943345)
• 00: Full (e802fe62)

[MBaesken]

Seems you need to adjust the COPYRIGHT too, but otherwise looks good.

[RealCLanger]

Looks good, modulo copyright year. 😄

[dholmes-ora]

That fixes the observed problem, but I find the existing test logic somewhat awkward. You only need a finally block to ensure something happens if an exception occurred - but here if you get an exception why do we care about joining the vthread? We don't know what the exception was or what it implies about the state of the vthread.

Also using wait/notify without a loop on the wait and checking a state variable is not the correct usage pattern and would be affected by spurious wakeups.

[reinrich]

That fixes the observed problem, but I find the existing test logic somewhat awkward. You only need a finally block to ensure something happens if an exception occurred - but here if you get an exception why do we care about joining the vthread? We don't know what the exception was or what it implies about the state of the vthread.

The test should notify(). Without the virtual thread will not terminate and stay there until the vm exits. This would influence (new) test cases as heap dumps would accumulate virtual threads from previous tests.

Btw this is another reason to call System.gc() before every test for fixing https://bugs.openjdk.org/browse/JDK-8361827

join() is helpful to find issues like the observed problem and to make sure the virtual thread's gone.

Also using wait/notify without a loop on the wait and checking a state variable is not the correct usage pattern and would be affected by spurious wakeups.

I've added the loop.

[plummercj]

With your fix in place, I assume now if verifyHeapDump() throws an exception the test will still fail, but in a more timely and meaningful manner. Why is verifyHeapDump() throwing an exception in the first place. Is that something that still needs to be addressed?

[dholmes-ora]

Thanks for update.

Still not sure that forcing the join is necessarily desirable if there is an unexpected exception, but ...

[dholmes-ora]

Should not be public

[reinrich]

With your fix in place, I assume now if verifyHeapDump() throws an exception the test will still fail, but in a more timely and meaningful manner.

Yes, that's the intention.

Why is verifyHeapDump() throwing an exception in the first place. Is that something that still needs to be addressed?

In our local ci testing it throws OOM (even reproducibly on one machine). This is addressed with https://bugs.openjdk.org/browse/JDK-8361827.
I've drafted a dependent pr: #26260

[plummercj]

Looks good.

[reinrich]

Thanks for the reviews.
/integrate

[openjdk]

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

• 3bacf7e: 8361869: Tests which call ThreadController should mark as /native
• 4a351e3: 8361328: cds/appcds/dynamicArchive/TestAutoCreateSharedArchive.java archive timestamps comparison failed
• 189017f: 8361908: Mix and match of dead and valid exception handler leads to malformed class file
• ... and 46 more: https://git.openjdk.org/jdk/compare/e0245682c8d5a0daae055045c81248c12fb23c09...master

Your commit was automatically rebased without conflicts.

[openjdk]

@reinrich Pushed as commit 917d018.

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

[reinrich]

/backport :jdk25

[openjdk]

@reinrich the backport was successfully created on the branch backport-reinrich-917d0182-jdk25 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk25, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 917d0182 from the openjdk/jdk repository.

The commit being backported was authored by Richard Reingruber on 12 Jul 2025 and was reviewed by Chris Plummer, Christoph Langer and David Holmes.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk:

$ git fetch https://github.com/openjdk-bots/jdk.git backport-reinrich-917d0182-jdk25:backport-reinrich-917d0182-jdk25
$ git checkout backport-reinrich-917d0182-jdk25
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk.git backport-reinrich-917d0182-jdk25