Skip to content

Conversation

@nickita-khylkouski
Copy link
Contributor

Summary

This PR improves TestThread.tearDown() to try interrupt() before falling back to Thread.stop(), as suggested by @cpovirk in #7319.

Changes:

  • Call interrupt() first, allowing interruptible threads (like those in InterruptibleMonitorTest) to clean up gracefully
  • Wait DUE_DILIGENCE_MILLIS (100ms) for the thread to respond to the interrupt
  • Only fall back to reflective stop() if the thread is still alive after the interrupt attempt

Motivation

The existing implementation immediately tries Thread.stop() via reflection, which:

  1. Is deprecated and throws UnsupportedOperationException on Java 20+
  2. Doesn't give interruptible threads a chance to clean up gracefully

Many test threads (particularly in InterruptibleMonitorTest) are designed to respond to interrupts. By trying interrupt() first, we allow these threads to terminate cleanly without relying on the deprecated stop() method.

Behavior by Java Version

Java Version interrupt() stop() fallback Thread cleanup
8-19 ✅ Tried first ✅ Works if needed Always works
20-25 ✅ Tried first ❌ Throws UnsupportedOperationException Works for interruptible threads
26+ ✅ Tried first ❌ Method removed Works for interruptible threads

Testing

Verified locally that existing tests pass:

  • InterruptibleMonitorTest - 16 tests ✅
  • UninterruptibleMonitorTest - 16 tests ✅
  • UninterruptiblesTest - 49 tests ✅

Related Issues

Fixes #7319

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);

Choose a reason for hiding this comment

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

"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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! A couple of important points about context:

  1. This PR doesn't introduce Thread.stop() — the existing code already calls it. This PR actually reduces reliance on it by trying interrupt() first.

  2. The boolean polling pattern from Effective Java Item 78 can't apply here. TestThread is 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.

  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 no SLO

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lingering threads in multiple tests

3 participants