-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8289610: Degrade Thread.stop #10230
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
8289610: Degrade Thread.stop #10230
Changes from all commits
c31f867
540c729
cd5269b
380e404
dc6f03a
86424b6
49d0c6b
6bec7a4
76108ab
a00f38a
fe73620
93806f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,9 +129,7 @@ private static void runHooks() { | |
| } | ||
| if (hook != null) hook.run(); | ||
| } catch (Throwable t) { | ||
| if (t instanceof ThreadDeath td) { | ||
| throw td; | ||
| } | ||
| // ignore | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again change of behaviour if TD originates from debugger. |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1629,59 +1629,19 @@ private void exit() { | |
| } | ||
|
|
||
| /** | ||
| * Forces the thread to stop executing. | ||
| * <p> | ||
| * If there is a security manager installed, its {@code checkAccess} | ||
| * method is called with {@code this} | ||
| * as its argument. This may result in a | ||
| * {@code SecurityException} being raised (in the current thread). | ||
| * <p> | ||
| * If this thread is different from the current thread (that is, the current | ||
| * thread is trying to stop a thread other than itself), the | ||
| * security manager's {@code checkPermission} method (with a | ||
| * {@code RuntimePermission("stopThread")} argument) is called in | ||
| * addition. | ||
| * Again, this may result in throwing a | ||
| * {@code SecurityException} (in the current thread). | ||
| * <p> | ||
| * The thread represented by this thread is forced to stop whatever | ||
| * it is doing abnormally and to throw a newly created | ||
| * {@code ThreadDeath} object as an exception. | ||
| * <p> | ||
| * It is permitted to stop a thread that has not yet been started. | ||
| * If the thread is eventually started, it immediately terminates. | ||
| * <p> | ||
| * An application should not normally try to catch | ||
| * {@code ThreadDeath} unless it must do some extraordinary | ||
| * cleanup operation (note that the throwing of | ||
| * {@code ThreadDeath} causes {@code finally} clauses of | ||
| * {@code try} statements to be executed before the thread | ||
| * officially terminates). If a {@code catch} clause catches a | ||
| * {@code ThreadDeath} object, it is important to rethrow the | ||
| * object so that the thread actually terminates. | ||
| * <p> | ||
| * The top-level error handler that reacts to otherwise uncaught | ||
| * 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}. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using an implSpec tag here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| * | ||
| * @throws SecurityException if the current thread cannot | ||
| * modify this thread. | ||
| * @throws UnsupportedOperationException if invoked on a virtual thread | ||
| * @see #interrupt() | ||
| * @see #checkAccess() | ||
| * @see ThreadDeath | ||
| * @see ThreadGroup#uncaughtException(Thread,Throwable) | ||
| * @see SecurityManager#checkAccess(Thread) | ||
| * @see SecurityManager#checkPermission | ||
| * @deprecated This method is inherently unsafe. Stopping a thread with | ||
| * Thread.stop causes it to unlock all of the monitors that it | ||
| * has locked (as a natural consequence of the unchecked | ||
| * {@code ThreadDeath} exception propagating up the stack). If | ||
| * @throws UnsupportedOperationException always | ||
| * | ||
| * @deprecated This method was originally specified to "stop" a victim | ||
| * thread by causing the victim thread 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 | ||
| * other threads, potentially resulting in arbitrary behavior. Many | ||
| * uses of {@code stop} should be replaced by code that simply | ||
| * an inconsistent state, the damaged objects became visible to | ||
| * other threads, potentially resulting in arbitrary behavior. | ||
| * Usages of {@code stop} should be replaced by code that simply | ||
| * modifies some variable to indicate that the target thread should | ||
| * stop running. The target thread should check this variable | ||
| * regularly, and return from its run method in an orderly fashion | ||
|
|
@@ -1695,26 +1655,7 @@ private void exit() { | |
| */ | ||
| @Deprecated(since="1.2", forRemoval=true) | ||
| public final void stop() { | ||
| @SuppressWarnings("removal") | ||
| SecurityManager security = System.getSecurityManager(); | ||
| if (security != null) { | ||
| checkAccess(); | ||
| if (this != Thread.currentThread()) { | ||
| security.checkPermission(SecurityConstants.STOP_THREAD_PERMISSION); | ||
| } | ||
| } | ||
|
|
||
| if (isVirtual()) | ||
| throw new UnsupportedOperationException(); | ||
|
|
||
| // A zero status value corresponds to "NEW", it can't change to | ||
| // not-NEW because we hold the lock. | ||
| if (holder.threadStatus != 0) { | ||
| resume(); // Wake up thread if it was suspended; no-op otherwise | ||
| } | ||
|
|
||
| // The VM can handle all thread states | ||
| stop0(new ThreadDeath()); | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3094,7 +3035,6 @@ static void setHeadStackableScope(StackableScope scope) { | |
|
|
||
| /* Some private helper methods */ | ||
| private native void setPriority0(int newPriority); | ||
| private native void stop0(Object o); | ||
| private native void suspend0(); | ||
| private native void resume0(); | ||
| private native void interrupt0(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 1995, 2019, Oracle and/or its affiliates. All rights reserved. | ||
| * Copyright (c) 1995, 2022, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
|
|
@@ -26,26 +26,19 @@ | |
| package java.lang; | ||
|
|
||
| /** | ||
| * An instance of {@code ThreadDeath} is thrown in the victim thread | ||
| * when the (deprecated) {@link Thread#stop()} method is invoked. | ||
| * An instance of {@code ThreadDeath} was originally specified to be thrown | ||
| * by a victim thread when "stopped" with {@link Thread#stop()}. | ||
|
Comment on lines
+29
to
+30
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this have always mentioned the possibility of TD coming from a debugger as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| * | ||
| * <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 victim | ||
| * thread by causing the victim thread 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, for removal. | ||
| * | ||
| * @since 1.0 | ||
| */ | ||
|
|
||
| @Deprecated(since="20", forRemoval=true) | ||
| public class ThreadDeath extends Error { | ||
| @java.io.Serial | ||
| private static final long serialVersionUID = -4417128565033088268L; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -674,12 +674,9 @@ private void list(Map<ThreadGroup, List<Thread>> map, PrintStream out, int inden | |
| * uncaught exception handler} installed, and if so, its | ||
| * {@code uncaughtException} method is called with the same | ||
| * two arguments. | ||
| * <li>Otherwise, this method determines if the {@code Throwable} | ||
| * argument is an instance of {@link ThreadDeath}. If so, nothing | ||
| * special is done. Otherwise, a message containing the | ||
| * thread's name, as returned from the thread's {@link | ||
| * Thread#getName getName} method, and a stack backtrace, | ||
| * using the {@code Throwable}'s {@link | ||
| * <li>Otherwise, a message containing the thread's name, as returned | ||
| * from the thread's {@link Thread#getName getName} method, and a | ||
| * stack backtrace, using the {@code Throwable}'s {@link | ||
| * Throwable#printStackTrace() printStackTrace} method, is | ||
| * printed to the {@linkplain System#err standard error stream}. | ||
| * </ul> | ||
|
|
@@ -699,9 +696,8 @@ public void uncaughtException(Thread t, Throwable e) { | |
| Thread.getDefaultUncaughtExceptionHandler(); | ||
| if (ueh != null) { | ||
| ueh.uncaughtException(t, e); | ||
| } else if (!(e instanceof ThreadDeath)) { | ||
| System.err.print("Exception in thread \"" | ||
| + t.getName() + "\" "); | ||
| } else { | ||
| System.err.print("Exception in thread \"" + t.getName() + "\" "); | ||
|
Comment on lines
+699
to
+700
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again change in behaviour. |
||
| e.printStackTrace(System.err); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| <!doctype html> | ||
| <!-- | ||
| Copyright (c) 2005, 2019, Oracle and/or its affiliates. All rights reserved. | ||
| Copyright (c) 2005, 2022, Oracle and/or its affiliates. All rights reserved. | ||
| DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| This code is free software; you can redistribute it and/or modify it | ||
|
|
@@ -31,31 +31,32 @@ | |
| <body> | ||
| <h1>Java Thread Primitive Deprecation</h1> | ||
| <hr> | ||
| <h2>Why is <code>Thread.stop</code> deprecated?</h2> | ||
| <p>Because it is inherently unsafe. Stopping a thread causes it to | ||
| unlock all the monitors that it has locked. (The monitors are | ||
| unlocked as the <code>ThreadDeath</code> exception propagates up | ||
| <h2>Why is <code>Thread.stop</code> deprecated and the ability to | ||
| stop a thread removed?</h2> | ||
| <p>Because it was inherently unsafe. Stopping a thread caused it to | ||
| unlock all the monitors that it had locked. (The monitors were | ||
|
Comment on lines
+36
to
+37
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| unlocked as the <code>ThreadDeath</code> exception propagated up | ||
| the stack.) If any of the objects previously protected by these | ||
| monitors were in an inconsistent state, other threads may now view | ||
| monitors were in an inconsistent state, other threads may have viewed | ||
| these objects in an inconsistent state. Such objects are said to be | ||
| <i>damaged</i>. When threads operate on damaged objects, arbitrary | ||
| behavior can result. This behavior may be subtle and difficult to | ||
| detect, or it may be pronounced. Unlike other unchecked exceptions, | ||
| <code>ThreadDeath</code> kills threads silently; thus, the user has | ||
| no warning that his program may be corrupted. The corruption can | ||
| <code>ThreadDeath</code> killed threads silently; thus, the user had | ||
| no warning that their program may be corrupted. The corruption could | ||
| manifest itself at any time after the actual damage occurs, even | ||
| hours or days in the future.</p> | ||
| <hr> | ||
| <h2>Couldn't I just catch the <code>ThreadDeath</code> exception | ||
| and fix the damaged object?</h2> | ||
| <h2>Couldn't I have just caught <code>ThreadDeath</code> and fixed | ||
| the damaged object?</h2> | ||
| <p>In theory, perhaps, but it would <em>vastly</em> complicate the | ||
| task of writing correct multithreaded code. The task would be | ||
| nearly insurmountable for two reasons:</p> | ||
| <ol> | ||
| <li>A thread can throw a <code>ThreadDeath</code> exception | ||
| <li>A thread could throw a <code>ThreadDeath</code> exception | ||
| <i>almost anywhere</i>. All synchronized methods and blocks would | ||
| have to be studied in great detail, with this in mind.</li> | ||
| <li>A thread can throw a second <code>ThreadDeath</code> exception | ||
| <li>A thread could throw a second <code>ThreadDeath</code> exception | ||
| while cleaning up from the first (in the <code>catch</code> or | ||
| <code>finally</code> clause). Cleanup would have to be repeated till | ||
| it succeeded. The code to ensure this would be quite complex.</li> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ThreadDeath originates from the debugger then this is now a change in behaviour.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Alan on this. First, use and handling of
ThreadDeathis 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
ThreadDeathhandling occurs when programs throwThreadDeathexplicitly, that is, not usingThread.stop. This does seem to occur infrequently "in the wild" (we'll fix the jshell case). The point of throwingThreadDeathexplicitly is to take advantage of special-case handling ofThreadDeaththat 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will defer to your views here. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'll get the CSR moving again and we'll try to get this done.