8289610: Degrade Thread.stop #10230
8289610: Degrade Thread.stop #10230
Conversation
|
/csr |
|
👋 Welcome back alanb! A progress list of the required criteria for merging this PR into |
|
@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request. @AlanBateman please create a CSR request for issue JDK-8289610 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
|
@AlanBateman The following labels will be automatically applied to this pull request:
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
added
serviceability
| /** | ||
| * An instance of {@code ThreadDeath} is thrown in the victim thread | ||
| * when the (deprecated) {@link Thread#stop()} method is invoked. | ||
| * This error is no longer thrown. | ||
| * | ||
| * <p>An application should catch instances of this class only if it | ||
| * must clean up after being terminated asynchronously. If | ||
| * {@code ThreadDeath} is caught by a method, it is important that it | ||
| * be rethrown so that the thread actually dies. | ||
| * | ||
| * <p>The {@linkplain ThreadGroup#uncaughtException top-level error | ||
| * handler} does not print out a message if {@code ThreadDeath} is | ||
| * never caught. | ||
| * | ||
| * <p>The class {@code ThreadDeath} is specifically a subclass of | ||
| * {@code Error} rather than {@code Exception}, even though it is a | ||
| * "normal occurrence", because many applications catch all | ||
| * occurrences of {@code Exception} and then discard the exception. | ||
| * @deprecated {@link Thread#stop()} was originally specified to "stop" a | ||
| * thread by causing it to throw a {@code ThreadDeath}. It was inherently | ||
| * unsafe and deprecated in an early JDK release. The ability to "stop" a | ||
| * thread with {@code Thread.stop} has been removed and the {@code | ||
| * Thread.stop} method changed to throw an exception. Consequently, | ||
| * {@code ThreadDeath} is also deprecated and may be deprecated | ||
| * for removal in a future release. | ||
| * | ||
| * @since 1.0 | ||
| */ | ||
|
|
||
| @Deprecated(since="20") | ||
| public class ThreadDeath extends Error { |
There was a problem hiding this comment.
Actually, ThreadDeath is thrown manually by JShell instrumentation code since JDK‑8289613 (GH‑10166)1.
Footnotes
Webrevs
|
| * Thread.stop causes it to unlock all of the monitors that it | ||
| * has locked (as a natural consequence of the unchecked | ||
| * @throws UnsupportedOperationException | ||
| * always |
There was a problem hiding this comment.
fold with previous line.
There was a problem hiding this comment.
fold with previous line.
Done.
|
What about also removing and undocumenting |
|
There is a bunch of VM code for this too. Should we clean that up separately? |
Yes chatted with @dholmes-ora about this recently and he suggested separating out so there is a follow-on JBS issue created for that. Aside from JVM_StopThread, I suspect code that deals with pending exceptions before a thread start can go away as the JVM TI StopThread can't cause an async exception on an unstarted thread. I'm hoping Thread.stillborn can go away too. |
Sounds good. |
Ah yes, the table RuntimePermission can be updated as part of this (and the corresponding constant in sun/security/util/SecurityConstants). |
|
The change looks good. There are other tests that reference |
I've been trying to keep the test cleanup separate if possible, only because the changes become unwieldy. Aside from jshell, the tests that used Thread.stop have already been changed or removed in advance of this PR. The tests that have catch blocks for ThreadDeath will work as before as there is no Thread.stop. There are one or two tests that will need special attention - the reactivestreams-tck tests that you listed is 3rd party code and we might decide to do nothing there. |
|
The changes to the |
| @@ -2666,8 +2666,6 @@ private void invokeConfigurationListeners() { | |||
| for (Runnable c : listeners.values().toArray(new Runnable[0])) { | |||
| try { | |||
| c.run(); | |||
| } catch (ThreadDeath death) { | |||
| throw death; | |||
There was a problem hiding this comment.
In theory, before this change, some (odd/weird) application code which was registered as a listener could throw a ThreadDeath and we would then abort any other listeners from being run. With this change that would no longer be that case. However, I think that is fine, since the API doc of LogManager.addConfigurationListener() already states:
* It is recommended that listeners do not throw errors or exceptions.
*
* If a listener terminates with an uncaught error or exception then
* the first exception will be propagated to the caller of
* {@link #readConfiguration()} (or {@link #readConfiguration(java.io.InputStream)})
* after all listeners have been invoked.
Mandy asked about test cleanup too. I'm hoping that we will do the test cleanup separately, only to avoid the changes being too unwieldly. |
Sorry, I missed your previous response to that similar question. Doing those changes separately sounds fine to me. |
| * @deprecated This method was originally specified to "stop" a thread | ||
| * by causing it to throw a {@link ThreadDeath}. It was inherently | ||
| * unsafe. Stopping a thread caused it to unlock all of the | ||
| * monitors that it had locked (as a natural consequence of the | ||
| * {@code ThreadDeath} exception propagating up the stack). If | ||
| * any of the objects previously protected by these monitors were in | ||
| * an inconsistent state, the damaged objects become visible to |
There was a problem hiding this comment.
s/become/became/
(because you are writing in past tense now).
There was a problem hiding this comment.
Hi Alan,
Good to see this finally become non-functional but as long as the debugger/JVMTI can trigger throwing an async ThreadDeath then I have to question the potential change in behaviour introduced by removing all the catch (ThreadDeath td) logic in various places.
| // evaluate possible precedence of flushException over closeException | ||
| if ((flushException instanceof ThreadDeath) && | ||
| !(closeException instanceof ThreadDeath)) { |
There was a problem hiding this comment.
If the ThreadDeath originates from the debugger then this is now a change in behaviour.
There was a problem hiding this comment.
Good to see this finally become non-functional but as long as the debugger/JVMTI can trigger throwing an async ThreadDeath then I have to question the potential change in behaviour introduced by removing all the
catch (ThreadDeath td)logic in various places.
I've gone through this a few times too and concluded it would be better to remove these untestable code paths. The debugger scenario is more like what used to be Thread.stop(Throwable). It can be used to throw any exception or error, e.g. ask the debugger to throw IllegalArgumentException or SQLException when I'm at this breakpoint so I can see how the code behaves. Yes, the user could ask the debugger to throw java.lang.ThreadDeath when prompted but it's not really interesting now because code won't get this error outside of a debugger. We could leave the special handing but we have no way to test is and we'll need to the remove the special handling when TD is removed. As a general point, the special casing of TD is a bit inconsistent and more likely to be seen in older code rather newer code. A static analysis of 24M classes found very few usages in 3rd party libraries.
There was a problem hiding this comment.
I agree with Alan on this. First, use and handling of ThreadDeath is quite rare; see my analysis of the corpus search results in the bug report. Second, while this does change the behavior in the debugger case, I'm hard-pressed to see how anybody is relying on such behavior. And maybe somebody somewhere is indeed relying on this behavior, but it doesn't seem to me this behavior is guaranteed by any specifications.
A more likely way that programs could observe changes in ThreadDeath handling occurs when programs throw ThreadDeath explicitly, that is, not using Thread.stop. This does seem to occur infrequently "in the wild" (we'll fix the jshell case). The point of throwing ThreadDeath explicitly is to take advantage of special-case handling of ThreadDeath that might occur higher in the call stack. Such special-casing was at one time an accepted exception handing idiom, but it's essentially disused. I don't think the JDK needs to continue to support this old idiom.
There was a problem hiding this comment.
Okay I will defer to your views here. Thanks.
There was a problem hiding this comment.
Okay I will defer to your views here. Thanks.
Thank you, I'll get the CSR moving again and we'll try to get this done.
| if (t instanceof ThreadDeath td) { | ||
| throw td; | ||
| } | ||
| // ignore |
There was a problem hiding this comment.
Again change of behaviour if TD originates from debugger.
| * An instance of {@code ThreadDeath} was originally specified to be thrown | ||
| * by a victim thread when "stopped" with {@link Thread#stop()}. |
There was a problem hiding this comment.
Should this have always mentioned the possibility of TD coming from a debugger as well?
There was a problem hiding this comment.
Should this have always mentioned the possibility of TD coming from a debugger as well?
The debugger can cause any error or exception to be thrown so I don't think we should put anything in the TD spec about this.
| } else { | ||
| System.err.print("Exception in thread \"" + t.getName() + "\" "); |
| <p>Because it was inherently unsafe. Stopping a thread caused it to | ||
| unlock all the monitors that it had locked. (The monitors were |
There was a problem hiding this comment.
Just an aside but this rationale was always a significant under-statement as async-exceptions are inherently unsafe even if no concurrency or monitors are involved.
| @@ -167,11 +167,6 @@ public final void run() { | |||
| break; | |||
| } | |||
| } | |||
| } catch (ThreadDeath td) { | |||
| @@ -202,10 +202,6 @@ void pumpOneEventForFilters(int id) { | |||
|
|
|||
| eq.dispatchEvent(event); | |||
| } | |||
| catch (ThreadDeath death) { | |||
| * exceptions does not print out a message or otherwise notify the | ||
| * application if the uncaught exception is an instance of | ||
| * {@code ThreadDeath}. | ||
| * Throws {@code UnsupportedOperationException}. |
There was a problem hiding this comment.
Consider using an implSpec tag here.
There was a problem hiding this comment.
Consider using an implSpec tag here.
implSpec is usually for default methods or overrideable methods. In this case, Thread.stop is final so code that extends Thread can't override and change the behavior. So it's not clear to me that implSpec would be useful as it would duplicate the description in the generated javadoc.
|
@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: 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 65 new commits pushed to the
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 |
|
/integrate |
|
Going to push as commit acd5bcf.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@AlanBateman Pushed as commit acd5bcf. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Degrade Thread.stop to throw UOE unconditionally, deprecate ThreadDeath for removal, and remove the remaining special handling of ThreadDeath from the JDK.
Thread.stop is inherently unsafe and has been deprecated since JDK 1.2 (1998) with a link to a supplementary page that explains the rationale. Some of the API surface has already been degraded or removed: Thread.stop(Throwable) was degraded to throw UOE in Java 8 and removed in Java 11, and ThreadGroup.stop was degraded to throw UOE in Java 19. As of Java 19, the no-arg Thread.stop continues to work as before for platform threads but throws UOE for virtual threads. The next step in the glacial pace removal is the degrading of the no-arg Thread.stop method to throw UOE for all threads.
To keep things manageable, the change proposed here leaves JVM_StopThread in place. A separate issue will remove it and do other cleanup/removal in the VM. We have another JBS issue for the updates to the JLS and JVMS where asynchronous exceptions are defined. There is also some remaining work on a test class used by 6 jshell tests - if they aren't done in time then we will temporarily exclude them.
The change here has no impact on the debugger APIs (JVM TI StopThread, JDWP ThreadReference/Stop, and JDI ThreadReference.stop). Debuggers can continue to cause threads to throw an asynchronous exception, as might be done when simulating code throwing an exception at some point in the code.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10230/head:pull/10230$ git checkout pull/10230Update a local copy of the PR:
$ git checkout pull/10230$ git pull https://git.openjdk.org/jdk pull/10230/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10230View PR using the GUI difftool:
$ git pr show -t 10230Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10230.diff