jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is

Open sspitsyn opened this issue 11 months ago • 17 comments

Please, review this fix correcting the JVMTI RawMonitorWait() implementation. The RawMonitorWait() is using the the jt->is_interrupted(true) to update the interrupt status of the interrupted waiting thread. The issue is that when it calls jt->is_interrupted(true) to fetch and clear the interrupt status of the virtual thread, this information is not propagated to the java.lang.Thread instance. In the VirtualThread class implementation the interrupt status for virtual thread is stored for both virtual and carrier threads. It is because the implementation of object monitors for virtual threads is based on pinning virtual threads, and so, always operates on carrier thread. The fix is to clear the interrupt status for both virtual and carrier java.lang.Thread instances.

Testing:

  • tested with new test hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor which is passed with the fix and failed without it
  • ran mach5 tiers 1-6

Progress

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

Issue

  • JDK-8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18093

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

Using diff file

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

Webrev

Link to Webrev Comment

sspitsyn avatar Mar 01 '24 23:03 sspitsyn

:wave: 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.

bridgekeeper[bot] avatar Mar 01 '24 23:03 bridgekeeper[bot]

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

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

openjdk[bot] avatar Mar 01 '24 23:03 openjdk[bot]

Webrevs

mlbridge[bot] avatar Mar 01 '24 23:03 mlbridge[bot]

I have to say that I don't understand how the behaviour of RawMonitorWait is any different to ObjectMonitor::wait when it comes to the use of the is_interrupted(true). ??? Is it simply that because we are in native code and we can immediately query the actual thread state that we can observe when the carrier and virtual thread states are transiently different?

dholmes-ora avatar Mar 05 '24 07:03 dholmes-ora

I have to say that I don't understand how the behaviour of RawMonitorWait is any different to ObjectMonitor::wait when it comes to the use of the is_interrupted(true). ??? Is it simply that because we are in native code and we can immediately query the actual thread state that we can observe when the carrier and virtual thread states are transiently different?

The behavior of ObjectMonitor::wait and RawMonitorWait is different. The Object.wait() throws the InterruptedException if it was interrupted. The RawMonitorWait clears the thread interrupt status and returns the error code JVMTI_ERROR_INTERRUPT.

Also, I guess, Alan partially answered this question here:

For Object.wait, the clearing of the interrupt status is done in the Java code, something that will change once Object.wait changes to freeze/unmount when not pinned.

sspitsyn avatar Mar 05 '24 19:03 sspitsyn

The Object.wait() throws the InterruptedException if it was interrupted.

And clears the interrupted status, just like RawMonitorWait.

plummercj avatar Mar 05 '24 21:03 plummercj

The behavior of ObjectMonitor::wait and RawMonitorWait is different. The Object.wait() throws the InterruptedException if it was interrupted. The RawMonitorWait clears the thread interrupt status and returns the error code JVMTI_ERROR_INTERRUPT.

That is not a significant/relevant difference as far as I can see. They both call thread->is_interrupted(true).

For Object.wait, the clearing of the interrupt status is done in the Java code,

Okay so that is where the carrier and virtual thread states get back in sync, and that is what is missing in the RawMonitorWait case. The proposed fix/change to is_interrupted is what threw me as that is the wrong place to make a change because it affects both cases. What is missing is an upcall from RawMonitorWait to some Java code to do the same job as the Object.wait Java code does.

dholmes-ora avatar Mar 05 '24 23:03 dholmes-ora

Added ObjectLocker of the interruptLock for sync to the JavaThread::is_interrupted().

sspitsyn avatar Mar 06 '24 09:03 sspitsyn

I think the updated implementation looks right. I think you can drop the catch of InterruptedException in Object.wait now. Easy to check this by running test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java as it has tests for interrupting Object.wait.

AlanBateman avatar Mar 06 '24 18:03 AlanBateman

I think you can drop the catch of InterruptedException in Object.wait now. Easy to check this by running test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java as it has tests for interrupting Object.wait.

Thanks. Fixed now. Tested with the test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java.

sspitsyn avatar Mar 07 '24 00:03 sspitsyn

Okay so that is where the carrier and virtual thread states get back in sync, and that is what is missing in the RawMonitorWait case. The proposed fix/change to is_interrupted is what threw me as that is the wrong place to make a change because it affects both cases. What is missing is an upcall from RawMonitorWait to some Java code to do the same job as the Object.wait Java code does.

There was a duplication in the Object.wait which has been just removed. Please, see the incremental update 07: Full - Incremental (923a3ff5). Now, both Object.wait and RawMonitorWait clear the interrupt status with the JavaThread::is_interrupted() only.

sspitsyn avatar Mar 07 '24 00:03 sspitsyn

Can you check if vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java is passing now? That test is a bit annoying in that it fails every time we touch j.l.Object so you might have to update BI04/bi04t002/newclass02/java.base/java/lang/Object.java again.

AlanBateman avatar Mar 07 '24 07:03 AlanBateman

Can you check if vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java is passing now? That test is a bit annoying in that it fails every time we touch j.l.Object so you might have to update BI04/bi04t002/newclass02/java.base/java/lang/Object.java again.

Checked, it is passed now.

sspitsyn avatar Mar 08 '24 05:03 sspitsyn

Can you check if vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002/TestDescription.java is passing now? That test is a bit annoying in that it fails every time we touch j.l.Object so you might have to update BI04/bi04t002/newclass02/java.base/java/lang/Object.java again.

Checked, it is passed now.

Can you look at it again? That test has a copy of Object.java so needs to be updated every time we touch Object.java. It's an annoying tax and I hope there is a JBS issue to replace that test. In this case, it will be benign because the instrumented version will just clear the interrupt status of both threads twice, but at the same time, it will be out of sync with Object.wait.

(This of course will not be necessary if the change is limited to JVMTI RawMonitorWait).

AlanBateman avatar Mar 08 '24 07:03 AlanBateman

Can you look at it again? That test has a copy of Object.java so needs to be updated every time we touch Object.java. It's an annoying tax and I hope there is a JBS issue to replace that test. In this case, it will be benign because the instrumented version will just clear the interrupt status of both threads twice, but at the same time, it will be out of sync with Object.wait.

This test is passing with removed the Object.wait catch (InterruptedException e) block in the test (instrumented version?) version of the Object.java.

I'm thinking if it makes sense to restore the removed catch (InterruptedException e) block in src/java.base/share/classes/java/lang/Object.java. It will make David more happy, and also remove a potential issue with the test vmTestbase/nsk/jvmti/scenarios/bcinstr/BI04/bi04t002. Clearing the interrupt status twice does not looks that bad.

sspitsyn avatar Mar 08 '24 13:03 sspitsyn

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

8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is

Reviewed-by: dholmes, alanb

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:

  • 053ff76e14046f796f6e10a9cb2ede1f1ae22ed6: 8308660: C2 compilation hits 'node must be dead' assert
  • e1b0af29e47b46879defce1fc44c30d4d50d0c31: 8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed
  • c0fc9563a707cb01db4baf3aebede5f6b3ea08d1: 8328275: CodeCache::print_internals should not be called in PRODUCT code
  • 4ef591f71f62ee6ea8a603ed7a3e568b348b2c16: 8326964: Remove Eclipse Shared Workspaces
  • ac5b6cb2d42bdb8fb1a110ad33411b50cff4ea61: 8327757: Convert javax/swing/JSlider/6524424/bug6524424.java applet to main
  • 652fb3aa48fdfe09e827c2a06f76b3a69c711b74: 8328154: Convert sun/java2d/loops/CopyAreaSpeed.java applet test to main
  • 9059727df135dc90311bd476124f090b5766092b: 8327182: Move serverAlias into the loop
  • 2dd5fba3bd37c577b8442b67a67dbcb22b9a530e: 8319889: Vector API tests trigger VM crashes with -XX:+StressIncrementalInlining
  • 3f2e849c54c2a9c55e3b5c9f5a6d3478b83144e3: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
  • c901da48e30d53cb8e4e3c1f0584c5f2d3d095f1: 8327098: GTest needs larger combination limit
  • ... and 45 more: https://git.openjdk.org/jdk/compare/2482a505e5c898cc6365aa4fb8ca3e8b758b3a97...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[bot] avatar Mar 13 '24 20:03 openjdk[bot]

I've restored the InterruptedException catch in the Object.wait. I believe, all the concerns have been resolved now.

sspitsyn avatar Mar 15 '24 09:03 sspitsyn

David and Alan, thank you for review!

sspitsyn avatar Mar 19 '24 08:03 sspitsyn

/integrate

sspitsyn avatar Mar 19 '24 08:03 sspitsyn

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

  • 053ff76e14046f796f6e10a9cb2ede1f1ae22ed6: 8308660: C2 compilation hits 'node must be dead' assert
  • e1b0af29e47b46879defce1fc44c30d4d50d0c31: 8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed
  • c0fc9563a707cb01db4baf3aebede5f6b3ea08d1: 8328275: CodeCache::print_internals should not be called in PRODUCT code
  • 4ef591f71f62ee6ea8a603ed7a3e568b348b2c16: 8326964: Remove Eclipse Shared Workspaces
  • ac5b6cb2d42bdb8fb1a110ad33411b50cff4ea61: 8327757: Convert javax/swing/JSlider/6524424/bug6524424.java applet to main
  • 652fb3aa48fdfe09e827c2a06f76b3a69c711b74: 8328154: Convert sun/java2d/loops/CopyAreaSpeed.java applet test to main
  • 9059727df135dc90311bd476124f090b5766092b: 8327182: Move serverAlias into the loop
  • 2dd5fba3bd37c577b8442b67a67dbcb22b9a530e: 8319889: Vector API tests trigger VM crashes with -XX:+StressIncrementalInlining
  • 3f2e849c54c2a9c55e3b5c9f5a6d3478b83144e3: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
  • c901da48e30d53cb8e4e3c1f0584c5f2d3d095f1: 8327098: GTest needs larger combination limit
  • ... and 45 more: https://git.openjdk.org/jdk/compare/2482a505e5c898cc6365aa4fb8ca3e8b758b3a97...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Mar 19 '24 08:03 openjdk[bot]

@sspitsyn Pushed as commit 6eea5d675566adca3fca88639008c6c0221450a4.

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

openjdk[bot] avatar Mar 19 '24 08:03 openjdk[bot]