8291456: com/sun/jdi/ClassUnloadEventTest.java failed with: Wrong number of class unload events: expected 10 got 4

[sspitsyn]

The JDI ClassUnloadEvent events are synthesized by the JDWP agent from the JVM TI ObjectFree events.
The JVM TI ObjectFree events are flushed when the JVM TI SetEvenNotificationMode is used to disable the ObjectFree events. It is not very helpful for JDWP agent as the ObjectFree events are always enabled.
The fix is to flush all pending ObjectFree events at the VM shutdown.

Testing:

All mach5 jobs with JVMTI/JDI tests and tiers 1-6 were successfully passed on 3 debug platforms.

---

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-8291456: com/sun/jdi/ClassUnloadEventTest.java failed with: Wrong number of class unload events: expected 10 got 4

Reviewers

• Chris Plummer (@plummercj - Reviewer)
• Alex Menkov (@alexmenkov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10736

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

Using diff file

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

[bridgekeeper]

👋 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]

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

• hotspot
• 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.

[mlbridge]

Webrevs

• 00: Full (238652e5)

[plummercj]

The changes look good. Did you first make sure you could reproduce the problem without the fix, and then verify with the fix?

[sspitsyn]

Yes, I was able to reproduce the original issue without my fix.
It is not reproducible anymore with the fix.

[dholmes-ora]

The agent will be racing against VM termination and will need to be resilient to potentially getting "wrong phase" errors, or similar.

[sspitsyn]

The agent will be racing against VM termination and will need to be
resilient to potentially getting "wrong phase" errors, or similar.

I'm not sure what you mean.
There is always this kind of race but this fix does not make it any worse.
Please, note, the ObjectFree events in this fix are flushed/posted synchronously before the VMDeath event.
The JDWP agent does all needed to be resistant to the WRONG_PHASE errors.
If you talk about all other JVM TI agents then it is a common problem which does not have a solution yet.
It means, all JVM TI agents should check for the WRONG_PHASE errors and process them as needed.

[openjdk]

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

8291456: com/sun/jdi/ClassUnloadEventTest.java failed with: Wrong number of class unload events: expected 10 got 4

Reviewed-by: cjplummer, amenkov

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

• 8d4c077: 8295302: Do not use ArrayList when LambdaForm has a single ClassData
• 017e798: 8293939: Move continuation_enter_setup and friends
• f872467: 8255746: Make PrintCompilation available on a per method level
• 388a56e: 8294467: Fix sequence-point warnings in Hotspot
• ceb5b08: 8294468: Fix char-subscripts warnings in Hotspot
• 7b1c676: 8295662: jdk/incubator/vector tests fail "assert(VM_Version::supports_avx512vlbw()) failed"
• 5eaf568: 8295668: validate-source failure after JDK-8290011
• e238920: 8295372: CompactNumberFormat handling of number one with decimal part
• a5f6e31: 8295456: (ch) sun.nio.ch.Util::checkBufferPositionAligned gives misleading/incorrect error
• e27bea0: 8290011: IGV: Remove dead code and cleanup
• ... and 45 more: https://git.openjdk.org/jdk/compare/21a825e059170e3a069b9f0982737c5839e6dae2...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.

[dholmes-ora]

There is always this kind of race but this fix does not make it any worse.

Before this fix events were not getting flushed so at VM exit the agent was idle with respect to those events and the test failed because events were missing. Now you are flushing those events are VM exit and so the agent will now be very active at VM exit, and racing against the rest of the termination sequence.

[dholmes-ora]

Please, note, the ObjectFree events in this fix are flushed/posted synchronously before the VMDeath event.

Do you mean that before the flush operation returns all events are guaranteed to have been processed?

[sspitsyn]

Do you mean that before the flush operation returns all events are guaranteed to have been processed?

Exactly. And it happens before the VMDeath event is posted.

[dholmes-ora]

Ah sorry. I was thinking the events were flushed to a queue and the processed later.

[sspitsyn]

No problem. Sorry, it was not clear.

[sspitsyn]

Chris and Alex, thank you for review!

[sspitsyn]

/integrate

[openjdk]

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

• 8d4c077: 8295302: Do not use ArrayList when LambdaForm has a single ClassData
• 017e798: 8293939: Move continuation_enter_setup and friends
• f872467: 8255746: Make PrintCompilation available on a per method level
• 388a56e: 8294467: Fix sequence-point warnings in Hotspot
• ceb5b08: 8294468: Fix char-subscripts warnings in Hotspot
• 7b1c676: 8295662: jdk/incubator/vector tests fail "assert(VM_Version::supports_avx512vlbw()) failed"
• 5eaf568: 8295668: validate-source failure after JDK-8290011
• e238920: 8295372: CompactNumberFormat handling of number one with decimal part
• a5f6e31: 8295456: (ch) sun.nio.ch.Util::checkBufferPositionAligned gives misleading/incorrect error
• e27bea0: 8290011: IGV: Remove dead code and cleanup
• ... and 45 more: https://git.openjdk.org/jdk/compare/21a825e059170e3a069b9f0982737c5839e6dae2...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sspitsyn Pushed as commit c5e0464.

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