Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon #1318

Closed
wants to merge 6 commits into from

Conversation

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 19, 2020

This change terminally deprecates the following methods defined by java.lang.ThreadGroup

  • stop
  • destroy
  • isDestroyed
  • setDaemon
  • isDaemon

The stop method has been deprecated since=1.2 because it is inherently unsafe. It is time to terminally deprecate this method so it can be removed in a future release. Thread.stop will be examined in a separate issue.

The destroy, isDestroyed, setDaemon, isDaemon methods support the mechanism to explicitly or automatically destroy a thread group. As detailed in JDK-8252885, the mechanism to destroy thread groups is flawed and racy. Furthermore, this mechanism inhibits efforts to drop the reference from a thread group to its threads (so that thread creation, starting and termination do not need to coordinate with their thread group). These methods should be terminally deprecated so they can be degraded in a future release and eventually removed.

CSR with more information: https://bugs.openjdk.java.net/browse/JDK-8256644


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (6/6 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1318/head:pull/1318
$ git checkout pull/1318

@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 19, 2020

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 19, 2020

👋 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 openjdk bot added the csr label Nov 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 19, 2020

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

@openjdk
Copy link

@openjdk openjdk bot commented Nov 19, 2020

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

  • awt
  • core-libs

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 openjdk bot added the kulla label Nov 19, 2020
mrserb
mrserb approved these changes Nov 19, 2020
@mrserb
Copy link
Member

@mrserb mrserb commented Nov 19, 2020

The changes in the awt are trivial and look fine.

@AlanBateman AlanBateman marked this pull request as ready for review Nov 20, 2020
@openjdk openjdk bot added the rfr label Nov 20, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 20, 2020

Webrevs

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Nov 20, 2020

I think it would be useful in the javadoc of the RuntimePermission targets (stopThread, etc) to add a note/link that the corresponding method that the permission applies to is terminally deprecated. Something as simple as "Note that the Thread.stop() method is terminally deprecated and will be removed in a future release."

@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 20, 2020

I think it would be useful in the javadoc of the RuntimePermission targets (stopThread, etc) to add a note/link that the corresponding method that the permission applies to is terminally deprecated. Something as simple as "Note that the Thread.stop() method is terminally deprecated and will be removed in a future release."

We haven't changed Thread.stop here, I would like to get that done too but as separate issue because it will require wider discussion. The main thing for now is the prep work before proposing to shove ThreadGroup to the side.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Nov 20, 2020

I think it would be useful in the javadoc of the RuntimePermission targets (stopThread, etc) to add a note/link that the corresponding method that the permission applies to is terminally deprecated. Something as simple as "Note that the Thread.stop() method is terminally deprecated and will be removed in a future release."

We haven't changed Thread.stop here, I would like to get that done too but as separate issue because it will require wider discussion. The main thing for now is the prep work before proposing to shove ThreadGroup to the side.

Ok, but then how about putting a similar note in the javadoc for the RuntimePermission "modifyThreadGroup" target?

* operation and is inherently racy with respect to threads that
* have been created but have not started. The concept of daemon
* thread group that is automatically destroyed will be removed
* in a future release.
*/
Copy link
Contributor

@RogerRiggs RogerRiggs Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would switch the order of the sentences. Put the action first and the rationale second. $.02
(in all the @ deprecated javadoc).

@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 20, 2020

Ok, but then how about putting a similar note in the javadoc for the RuntimePermission "modifyThreadGroup" target?

The "modifyThread" and "modifyThreadGroup" permission targets list methods that have been terminally deprecated for some time, are you looking for both permission targets to be updated? I could imagine doing an overall here, also re-visiting SecurityManager checkAccess(ThreadGroup) but it feels like it's beyond the scope of the deprecation proposed here and would be better off being left to when we eventually degrade and/or remove these methods.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Nov 20, 2020

Ok, but then how about putting a similar note in the javadoc for the RuntimePermission "modifyThreadGroup" target?

The "modifyThread" and "modifyThreadGroup" permission targets list methods that have been terminally deprecated for some time, are you looking for both permission targets to be updated? I could imagine doing an overall here, also re-visiting SecurityManager checkAccess(ThreadGroup) but it feels like it's beyond the scope of the deprecation proposed here and would be better off being left to when we eventually degrade and/or remove these methods.

Ok, I see there is a broader context for my comment. I think then it makes sense to open a separate issue to specify these various RuntimePermission targets that are associated with deprecated APIs. In my mind, these permission targets are standard targets and should also be "deprecated" even if there is no annotation that can formally indicate that. For comparison, SecurityPermission puts these in a separate table in the class summary: https://download.java.net/java/early_access/jdk16/docs/api/java.base/java/security/SecurityPermission.html
If you agree, I can file an issue.

@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 20, 2020

If you agree, I can file an issue.

Yes, make sense to separate this out, esp. permission targets such as "stopThread" where all usages are in deprecated methods. However, I don't expect "modifyThreadGroup" would be deprecated, at least not yet, because there are usages in several methods that are not deprecated.

mrserb
mrserb approved these changes Nov 20, 2020
@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Nov 20, 2020

I think the current deprecation wording is actually too specific regarding the raciness between TG destruction and created-but-not-started threads. That's just one of the flaws of thread groups. In fact, I think there are enough weirdnesses and race conditions around all destruction-related operations of thread groups that the whole concept is fundamentally flawed. We should just say that. How about this:

ThreadGroup's destruction mechanisms are fundamentally flawed. Therefore, the ThreadGroup methods destroy(), isDestroyed(), setDaemon(), and isDaemon(), which relate to ThreadGroup destruction, have been deprecated and may be removed from a future version of the system.

I think there are too many subtle details to include a justification here about why TG destruction is fundamentally flawed, so we just have to assert that. Unfortunately the writeups in the JEP and CSR are in draft state so we can't link to them. Maybe when the JEP is published we can add a link to it from here later.

@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 22, 2020

Okay, I think I agree that the first sentence needs to be a bit more general so I've re-worded it. I used "inherently" rather than "fundamentally" to be consistent with the other deprecation text in Thread/ThreadGroup. If you are okay with the updated text then I'll transfer it to the CSR.

Copy link
Member

@stuart-marks stuart-marks left a comment

I'm ok with the wording "inherently flawed."

@openjdk openjdk bot removed the csr label Nov 24, 2020
@AlanBateman AlanBateman changed the title 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon Nov 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

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

8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon

Reviewed-by: serb, rriggs, iris, mchung, smarks

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:

  • 0d91f0a: 8252848: Optimize small primitive arrayCopy operations through partial inlining using AVX-512 masked instructions
  • 66943fe: 8256517: (ref) Reference.clear during reference processing may lose notification
  • 3c230b8: 8256993: Clarify Package::isSealed javadoc about package sealing vs sealed class or interface
  • 1b7a61f: 8254999: Move G1RemSetSamplingTask to more appropriate location
  • 695117f: 8255479: [aarch64] assert(src->section_index_of(target) == CodeBuffer::SECT_NONE) failed: sanity
  • dbfeb90: 8243559: Remove root certificates with 1024-bit keys
  • 2a1e9be: 8256364: vmTestbase/nsk/jvmti/scenarios/capability/CM01/cm01t002 failed with "assert(handle != __null) failed: JNI handle should not be null"
  • f1d6e8d: 8256387: Unexpected result if patching an entire instruction on AArch64
  • bd14274: 8256480: Refactor ObjectInputStream field reader implementation
  • 1c4c99e: 8256823: C2 compilation fails with "assert(isShiftCount(imm8 >> 1)) failed: illegal shift count"
  • ... and 45 more: https://git.openjdk.java.net/jdk/compare/1aa90ac6295e2cdda078eabf0a32e109c2e6c38f...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 openjdk bot added the ready label Nov 25, 2020
@AlanBateman
Copy link
Contributor Author

@AlanBateman AlanBateman commented Nov 25, 2020

/integrate

@openjdk openjdk bot closed this Nov 25, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

@AlanBateman Since your change was applied there have been 55 commits pushed to the master branch:

  • 0d91f0a: 8252848: Optimize small primitive arrayCopy operations through partial inlining using AVX-512 masked instructions
  • 66943fe: 8256517: (ref) Reference.clear during reference processing may lose notification
  • 3c230b8: 8256993: Clarify Package::isSealed javadoc about package sealing vs sealed class or interface
  • 1b7a61f: 8254999: Move G1RemSetSamplingTask to more appropriate location
  • 695117f: 8255479: [aarch64] assert(src->section_index_of(target) == CodeBuffer::SECT_NONE) failed: sanity
  • dbfeb90: 8243559: Remove root certificates with 1024-bit keys
  • 2a1e9be: 8256364: vmTestbase/nsk/jvmti/scenarios/capability/CM01/cm01t002 failed with "assert(handle != __null) failed: JNI handle should not be null"
  • f1d6e8d: 8256387: Unexpected result if patching an entire instruction on AArch64
  • bd14274: 8256480: Refactor ObjectInputStream field reader implementation
  • 1c4c99e: 8256823: C2 compilation fails with "assert(isShiftCount(imm8 >> 1)) failed: illegal shift count"
  • ... and 45 more: https://git.openjdk.java.net/jdk/compare/1aa90ac6295e2cdda078eabf0a32e109c2e6c38f...master

Your commit was automatically rebased without conflicts.

Pushed as commit 79e57ac.

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

openjdk-notifier bot referenced this issue Nov 25, 2020
… setDaemon and isDaemon

Reviewed-by: serb, rriggs, iris, mchung, smarks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment