TestThread: Try interrupt() before stop() in tearDown()#8169
TestThread: Try interrupt() before stop() in tearDown()#8169nickita-khylkouski wants to merge 2 commits intogoogle:masterfrom
Conversation
This allows interruptible threads (like those in InterruptibleMonitorTest) to clean up gracefully instead of relying solely on the deprecated Thread.stop() method. The implementation: 1. Calls interrupt() first 2. Waits DUE_DILIGENCE_MILLIS for the thread to respond 3. Falls back to reflective stop() only if the thread is still alive This addresses Issue google#7319 as suggested by cpovirk.
| // On Java 20+, stop() throws UnsupportedOperationException, so the thread will | ||
| // remain alive. This is unavoidable for uninterruptible threads. | ||
| try { | ||
| Thread.class.getMethod("stop").invoke(this); |
There was a problem hiding this comment.
"Do not use Thread.stop. A recommended way to stop one thread from another is to have the first thread poll a boolean field that is initially false but can be set to true by the second
thread to indicate that the first thread is to stop itself. Because reading and
writing a boolean field is atomic, some programmers dispense with synchronization when accessing the field:" - Item 78 of Effective Java programming (3rd edition).
There was a problem hiding this comment.
Thanks for the review! A couple of important points about context:
-
This PR doesn't introduce
Thread.stop()— the existing code already calls it. This PR actually reduces reliance on it by tryinginterrupt()first. -
The boolean polling pattern from Effective Java Item 78 can't apply here.
TestThreadis used in concurrency tests where threads are deliberately blocked inside synchronization primitives (Monitor.enter(),Condition.await(), etc.). A thread suspended in a native blocking call cannot poll a boolean field — it's not in a loop. -
The Guava maintainers already addressed this in the existing TODO comment, noting that some threads are in uninterruptible operations intentionally and there is no other way to clean them up. The long-term plan is to drop
stop()once Java 20+ is the minimum.
This PR implements the approach suggested by @cpovirk in #7319: try interrupt() first, keep stop() as a fallback only for older JDKs.
3 participants
Summary
This PR improves
TestThread.tearDown()to tryinterrupt()before falling back toThread.stop(), as suggested by @cpovirk in #7319.Changes:
interrupt()first, allowing interruptible threads (like those inInterruptibleMonitorTest) to clean up gracefullyDUE_DILIGENCE_MILLIS(100ms) for the thread to respond to the interruptstop()if the thread is still alive after the interrupt attemptMotivation
The existing implementation immediately tries
Thread.stop()via reflection, which:UnsupportedOperationExceptionon Java 20+Many test threads (particularly in
InterruptibleMonitorTest) are designed to respond to interrupts. By tryinginterrupt()first, we allow these threads to terminate cleanly without relying on the deprecatedstop()method.Behavior by Java Version
Testing
Verified locally that existing tests pass:
InterruptibleMonitorTest- 16 tests ✅UninterruptibleMonitorTest- 16 tests ✅UninterruptiblesTest- 49 tests ✅Related Issues
Fixes #7319