8249627: Degrade Thread.suspend and Thread.resume #10324

AlanBateman · 2022-09-18T16:32:31Z

Degrade Thread.suspend/resume to throw UOE unconditionally.

Another step in the removal of this deadlock prone mis-feature from the user-facing API. Thread.suspend/resume have been deprecated since JDK 1.2 (1998) and terminally deprecated since Java 14. ThreadGroup.suspend/resume were degraded to throw UOE in Java 19. As of Java 19, Thread.suspend/resume continues to work for platform threads but throws UOE for virtual threads. The next step is to degrade both methods to throw UOE for all threads. A corpus search of 19M classes in 113k JAR files found only 22 classes using these methods so this change is unlikely to be disruptive.

The change requires some minor adjustments to the JVM TI and JDWP specifications, and a minor update to the JDI docs.

Leonid Mesnik is working on PR10351 to remove/replace the last few usages of Thread.suspend/resume from the hotspot tests (most of these can use JVMTI SuspendThread/ResumeThread).

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
Change requires a CSR request to be approved

Issues

JDK-8249627: Degrade Thread.suspend and Thread.resume
JDK-8293975: Degrade Thread.suspend and Thread.resume (CSR)

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to d78ed990
Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to d35954e6
David Holmes (@dholmes-ora - Reviewer)
Jaikiran Pai (@jaikiran - Reviewer) ⚠️ Review applies to d78ed990

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10324

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

Using diff file

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

bridgekeeper · 2022-09-18T16:36:01Z

👋 Welcome back alanb! 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 · 2022-09-18T16:38:58Z

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

build
core-libs
hotspot
javadoc
security
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 · 2022-09-18T16:46:44Z

@AlanBateman Unknown command remove - for a list of valid commands use /help.

openjdk · 2022-09-18T16:46:58Z

@AlanBateman Unknown command remove - for a list of valid commands use /help.

openjdk · 2022-09-18T16:47:13Z

@AlanBateman Unknown command remove - for a list of valid commands use /help.

AlanBateman · 2022-09-18T16:48:14Z

/label remove build
/label remove javadoc
/label remove security

openjdk · 2022-09-18T16:49:16Z

@AlanBateman
The build label was successfully removed.

openjdk · 2022-09-18T16:50:57Z

@AlanBateman
The javadoc label was successfully removed.

openjdk · 2022-09-18T16:52:51Z

@AlanBateman
The security label was successfully removed.

AlanBateman · 2022-09-18T19:54:16Z

/csr

openjdk · 2022-09-18T19:55:18Z

@AlanBateman this pull request will not be integrated until the CSR request JDK-8293975 for issue JDK-8249627 has been approved.

stuart-marks · 2022-09-20T01:08:48Z

Code changes look fine, though I didn't look too closely at the JDWP and JVMTI stuff. Nice use of JUnit.

Not a big deal, but I could see leaving the links from Thread::suspend et. al. to the "Why deprecated?" doc, and then updating that doc to make it clear that the mechanisms have in fact been removed, but leaving the rationale that's there mostly in place.

Might also be useful in the CSR to re-emphasize that this does not affect the ability to suspend and resume threads through the debugging interfaces. Of course the specs in those areas need to be updated so they no longer deal with the case of a thread that's been suspended through the API, but the debugging mechanisms' functions themselves are unchanged.

mlbridge · 2022-09-21T13:47:45Z

Webrevs

02: Full (6aa38889)
01: Full (d35954e6)
00: Full (d78ed990)

plummercj

Changes look good.

sspitsyn

Looks good.
Thanks,
Serguei

jaikiran · 2022-09-22T07:08:45Z

Hello Alan, this PR deletes the CountStackFrames.java test case. Is that intentional? Looking at that test case it doesn't have any suspend/resume related testing and instead is just verifying that Thread.countStackFrames() throws the expected UnsupportedOperationException.

Edit: Please ignore what I said - I now see that these tests have been moved to a new test file.

dholmes-ora

One minor nit but otherwise looks good!

Thanks.

dholmes-ora · 2022-09-22T07:57:38Z

src/java.base/share/classes/java/lang/SecurityManager.java

@@ -494,7 +494,6 @@ private static ThreadGroup getRootGroup() {
     * calling thread is not allowed to modify the thread argument.
     * <p>
     * This method is invoked for the current security manager by the
-     * {@code stop}, {@code suspend}, {@code resume},


Why is stop being removed in this PR?

Why is stop being removed in this PR?

It's just the sequencing of the two PRs and avoid merge conflicts. The Thread.stop should really edit this sentence, then this PR will edit it again. It was simpler to just do the SM edits in one PR.

jaikiran

The java.base changes look fine to me.

AlanBateman · 2022-09-24T06:47:18Z

Leonid has integrated JDK-8289607 to remove the last usages of Thread.suspend/resume from the hotspot tests so I've refreshed this PR to drop the temporary excluding of these tests.

AlanBateman · 2022-09-27T11:36:31Z

Wakeup bot

openjdk · 2022-09-27T11:42:49Z

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

8249627: Degrade Thread.suspend and Thread.resume

Reviewed-by: cjplummer, sspitsyn, dholmes, jpai

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:

bc12e95: 8292969: Bad Thread Utilization in ForkJoinPool
dd51f7e: 8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop
14c6ac4: 8293998: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
02ea338: 8293887: AArch64 build failure with GCC 12 due to maybe-uninitialized warning in libfdlibm k_rem_pio2.c
49a7347: 8294408: Problemlist runtime/handshake/HandshakeSuspendExitTest.java
aa48705: 8289422: Fix and re-enable vector conditional move
1ddc92f: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
1e222bc: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
43eff2b: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
b88ee1e: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
... and 10 more: https://git.openjdk.org/jdk/compare/543851db926469df57a8f4a2bd3458349012145f...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.

AlanBateman · 2022-09-27T11:43:26Z

/integrate

openjdk · 2022-09-27T11:44:14Z

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

bc12e95: 8292969: Bad Thread Utilization in ForkJoinPool
dd51f7e: 8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop
14c6ac4: 8293998: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
02ea338: 8293887: AArch64 build failure with GCC 12 due to maybe-uninitialized warning in libfdlibm k_rem_pio2.c
49a7347: 8294408: Problemlist runtime/handshake/HandshakeSuspendExitTest.java
aa48705: 8289422: Fix and re-enable vector conditional move
1ddc92f: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
1e222bc: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
43eff2b: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
b88ee1e: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
... and 10 more: https://git.openjdk.org/jdk/compare/543851db926469df57a8f4a2bd3458349012145f...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-27T11:44:29Z

@AlanBateman Pushed as commit 1abf971.

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

AlanBateman added 2 commits September 17, 2022 19:19

Initial commit

5e47d6e

Tests

e24339e

openjdk bot added javadoc javadoc-dev@openjdk.org serviceability serviceability-dev@openjdk.org security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 18, 2022

openjdk bot removed the build build-dev@openjdk.org label Sep 18, 2022

openjdk bot removed the javadoc javadoc-dev@openjdk.org label Sep 18, 2022

openjdk bot removed the security security-dev@openjdk.org label Sep 18, 2022

openjdk bot added the csr Pull request needs approved CSR before integration label Sep 18, 2022

AlanBateman added 3 commits September 21, 2022 13:19

Merge

238b37f

Keep link to threadPrimitiveDeprecation.html

0ed1fec

Fix typo in test comment

d78ed99

AlanBateman marked this pull request as ready for review September 21, 2022 13:24

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2022

plummercj approved these changes Sep 21, 2022

View reviewed changes

sspitsyn approved these changes Sep 21, 2022

View reviewed changes

lmesnik mentioned this pull request Sep 22, 2022

8289607: Change hotspot/jtreg tests to not use Thread.suspend/resume #10351

Closed

3 tasks

dholmes-ora approved these changes Sep 22, 2022

View reviewed changes

jaikiran approved these changes Sep 22, 2022

View reviewed changes

AlanBateman added 6 commits September 22, 2022 15:18

Merge

cfbb828

Update modifyThread and modifyThreadGroup targets

557e635

Merge

0040e0e

Update DegradedMethodsThrowUOE to test Thread.stop

d35954e

Merge

c5c32f3

Remove tests from exclude list

6aa3888

sspitsyn approved these changes Sep 23, 2022

View reviewed changes

dholmes-ora approved these changes Sep 26, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 27, 2022

openjdk bot added the ready Pull request is ready to be integrated label Sep 27, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 27, 2022

openjdk bot closed this Sep 27, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8249627: Degrade Thread.suspend and Thread.resume #10324

8249627: Degrade Thread.suspend and Thread.resume #10324

AlanBateman commented Sep 18, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

AlanBateman commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

AlanBateman commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

stuart-marks commented Sep 20, 2022

mlbridge bot commented Sep 21, 2022 •

edited

Loading

plummercj left a comment

sspitsyn left a comment

jaikiran commented Sep 22, 2022 •

edited

Loading

dholmes-ora left a comment

dholmes-ora Sep 22, 2022

AlanBateman Sep 22, 2022

jaikiran left a comment

AlanBateman commented Sep 24, 2022

AlanBateman commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

AlanBateman commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

8249627: Degrade Thread.suspend and Thread.resume #10324

8249627: Degrade Thread.suspend and Thread.resume #10324

Conversation

AlanBateman commented Sep 18, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

AlanBateman commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

AlanBateman commented Sep 18, 2022

openjdk bot commented Sep 18, 2022

stuart-marks commented Sep 20, 2022

mlbridge bot commented Sep 21, 2022 • edited Loading

Webrevs

plummercj left a comment

Choose a reason for hiding this comment

sspitsyn left a comment

Choose a reason for hiding this comment

jaikiran commented Sep 22, 2022 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Sep 22, 2022

Choose a reason for hiding this comment

AlanBateman Sep 22, 2022

Choose a reason for hiding this comment

jaikiran left a comment

Choose a reason for hiding this comment

AlanBateman commented Sep 24, 2022

AlanBateman commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

AlanBateman commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

openjdk bot commented Sep 27, 2022

AlanBateman commented Sep 18, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 21, 2022 •

edited

Loading

jaikiran commented Sep 22, 2022 •

edited

Loading