Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 20:12:31 GMT, Stuart Marks wrote: >> Marked as reviewed by mchung (Reviewer). > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 19:59:52 GMT, Mandy Chung wrote: >> Alan Bateman has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Fixed typo in @deprecated text >> - Merge >> - Update jshell class >> - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, >> setDaemon and isDaemon > > Marked as reviewed by mchung (Reviewer). 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. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:08:27 GMT, Alan Bateman wrote: >> 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 > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Fixed typo in @deprecated text > - Merge > - Update jshell class > - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, > setDaemon and isDaemon Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:08:27 GMT, Alan Bateman wrote: >> 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 > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Fixed typo in @deprecated text > - Merge > - Update jshell class > - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, > setDaemon and isDaemon Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:08:27 GMT, Alan Bateman wrote: >> 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 > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Fixed typo in @deprecated text > - Merge > - Update jshell class > - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, > setDaemon and isDaemon Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 16:05:24 GMT, Sean Mullan wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:50:11 GMT, Alan Bateman wrote: > > 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. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:27:14 GMT, Roger Riggs wrote: >> Alan Bateman has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Fixed typo in @deprecated text >> - Merge >> - Update jshell class >> - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, >> setDaemon and isDaemon > > Marked as reviewed by rriggs (Reviewer). > 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. - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 15:08:27 GMT, Alan Bateman wrote: >> 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 > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Fixed typo in @deprecated text > - Merge > - Update jshell class > - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, > setDaemon and isDaemon Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/java/lang/ThreadGroup.java line 194: > 192: * thread group that is automatically destroyed will be > removed > 193: * in a future release. > 194: */ I would switch the order of the sentences. Put the action first and the rationale second. $.02 (in all the @ deprecated javadoc). - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
On Fri, 20 Nov 2020 14:49:16 GMT, Alan Bateman wrote: > > 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? - PR: https://git.openjdk.java.net/jdk/pull/1318
Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]
> 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 Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Fixed typo in @deprecated text - Merge - Update jshell class - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, setDaemon and isDaemon - Changes: - all: https://git.openjdk.java.net/jdk/pull/1318/files - new: https://git.openjdk.java.net/jdk/pull/1318/files/c661861a..ca656ba1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1318=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1318=00-01 Stats: 99634 lines in 382 files changed: 45266 ins; 43186 del; 11182 mod Patch: https://git.openjdk.java.net/jdk/pull/1318.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1318/head:pull/1318 PR: https://git.openjdk.java.net/jdk/pull/1318
