Only create ThreadGroups if FailOnTimeout.lookForStuckThread is true. (#1691)

This reduces the differences (where possible) when tests are run with a timeout.
Creating a ThreadGroup can cause noticeable differences, for example in code
that uses java.beans.ThreadGroupContext.

Author: kcooney

doc/ReleaseNotes4.13.2.md

@@ -11,3 +11,20 @@ people reported problems that were caused by explicitly destroying the `ThreadGr
 
 In this change, the code was updated to call  `ThreadGroup.setDaemon(true)` instead of destroying the
 ThreadGroup.
+
+### [Pull request $1691:](https://github.com/junit-team/junit/pull/1691) Only create ThreadGroups if FailOnTimeout.lookForStuckThread is true.
+
+In JUnit 4.12 ([pull request #742](https://github.com/junit-team/junit4/pull/742)) the `Timeout`
+Rule was updated to optionally display the stacktrace of the thread that appears to be stuck
+(enabled on an opt-in basis by passing `true` to `Timeout.Builder.lookForStuckThread(boolean)`).
+When that change was made, time-limited tests were changed to start the new thread in a new
+`ThreadGroup`, even if the test did not call `lookForStuckThread()`. This subtle change in
+behavior resulted in visible behavior changes to some tests (for example, tests of code that uses
+`java.beans.ThreadGroupContext`).
+
+In this change, the code is updated to only create a new `ThreadGroup` if the caller calls
+`Timeout.Builder.lookForStuckThread(true)`. Tests with timeouts that do not make this call will
+behave as they did in JUnit 4.11 (and more similar to tests that do not have a timeout). This
+unfortunately could result in visible changes of tests written or updated since the 4.12
+release. If this change adversely affects your tests, you can create the `Timeout` rule via the
+builder and call `Timeout.Builder.lookForStuckThread(true)`.

src/main/java/org/junit/internal/runners/statements/FailOnTimeout.java

@@ -120,14 +120,7 @@ public FailOnTimeout build(Statement statement) {
     public void evaluate() throws Throwable {
         CallableStatement callable = new CallableStatement();
         FutureTask<Throwable> task = new FutureTask<Throwable>(callable);
-        ThreadGroup threadGroup = new ThreadGroup("FailOnTimeoutGroup");
-        if (!threadGroup.isDaemon()) {
-            try {
-                threadGroup.setDaemon(true);
-            } catch (SecurityException e) {
-                // Swallow the exception to keep the same behavior as in JUnit 4.12.
-            }
-        }
+        ThreadGroup threadGroup = threadGroupForNewThread();
         Thread thread = new Thread(threadGroup, task, "Time-limited test");
         thread.setDaemon(true);
         thread.start();
@@ -138,6 +131,31 @@ public void evaluate() throws Throwable {
         }
     }
 
+    private ThreadGroup threadGroupForNewThread() {
+        if (!lookForStuckThread) {
+            // Use the default ThreadGroup (usually the one from the current
+            // thread).
+            return null;
+        }
+
+        // Create the thread in a new ThreadGroup, so if the time-limited thread
+        // becomes stuck, getStuckThread() can find the thread likely to be the
+        // culprit.
+        ThreadGroup threadGroup = new ThreadGroup("FailOnTimeoutGroup");
+        if (!threadGroup.isDaemon()) {
+            // Mark the new ThreadGroup as a daemon thread group, so it will be
+            // destroyed after the time-limited thread completes. By ensuring the
+            // ThreadGroup is destroyed, any data associated with the ThreadGroup
+            // (ex: via java.beans.ThreadGroupContext) is destroyed.
+            try {
+                threadGroup.setDaemon(true);
+            } catch (SecurityException e) {
+                // Swallow the exception to keep the same behavior as in JUnit 4.12.
+            }
+        }
+        return threadGroup;
+    }
+
     /**
      * Wait for the test task, returning the exception thrown by the test if the
      * test failed, an exception indicating a timeout if the test timed out, or

src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java

@@ -8,32 +8,53 @@
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-import static org.junit.internal.runners.statements.FailOnTimeout.builder;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeTrue;
 
+import java.util.Arrays;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Test;
 import org.junit.function.ThrowingRunnable;
 import org.junit.internal.runners.statements.Fail;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
 import org.junit.runners.model.Statement;
 import org.junit.runners.model.TestTimedOutException;
 
 
 /**
  * @author Asaf Ary, Stefan Birkner
  */
+@RunWith(Parameterized.class)
 public class FailOnTimeoutTest {
     private static final long TIMEOUT = 100;
     private static final long DURATION_THAT_EXCEEDS_TIMEOUT = 60 * 60 * 1000; //1 hour
 
     private final TestStatement statement = new TestStatement();
 
-    private final FailOnTimeout failOnTimeout = builder().withTimeout(TIMEOUT, MILLISECONDS).build(statement);
+    private final boolean lookingForStuckThread;
+    private final FailOnTimeout failOnTimeout;
+
+    @Parameterized.Parameters(name = "lookingForStuckThread = {0}")
+    public static Iterable<Boolean> getParameters() {
+        return Arrays.asList(Boolean.TRUE, Boolean.FALSE);
+    }
+
+    public FailOnTimeoutTest(Boolean lookingForStuckThread) {
+        this.lookingForStuckThread = lookingForStuckThread;
+        this.failOnTimeout = builder().withTimeout(TIMEOUT, MILLISECONDS).build(statement);
+    }
+
+    private FailOnTimeout.Builder builder() {
+        return FailOnTimeout.builder().withLookingForStuckThread(lookingForStuckThread);
+    }
 
     @Test
     public void throwsTestTimedOutException() {
@@ -212,14 +233,17 @@ private void notTheRealCauseOfTheTimeout() {
     }
 
     @Test
-    public void threadGroupNotLeaked() throws Throwable {
+    public void lookingForStuckThread_threadGroupNotLeaked() throws Throwable {
+        assumeTrue(lookingForStuckThread);
         final AtomicReference<ThreadGroup> innerThreadGroup = new AtomicReference<ThreadGroup>();
         final AtomicReference<Thread> innerThread = new AtomicReference<Thread>();
+        final ThreadGroup outerThreadGroup = currentThread().getThreadGroup();
         ThrowingRunnable runnable = evaluateWithDelegate(new Statement() {
             @Override
             public void evaluate() {
                 innerThread.set(currentThread());
                 ThreadGroup group = currentThread().getThreadGroup();
+                assertNotSame("inner thread should use a different thread group", outerThreadGroup, group);
                 innerThreadGroup.set(group);
                 assertTrue("the 'FailOnTimeoutGroup' thread group should be a daemon thread group", group.isDaemon());
             }
@@ -231,4 +255,19 @@ public void evaluate() {
         innerThread.get().join();
         assertTrue("the 'FailOnTimeoutGroup' thread group should be destroyed after running the test", innerThreadGroup.get().isDestroyed());
     }
+
+    @Test
+    public void notLookingForStuckThread_usesSameThreadGroup() throws Throwable {
+        assumeFalse(lookingForStuckThread);
+        final ThreadGroup outerThreadGroup = currentThread().getThreadGroup();
+        ThrowingRunnable runnable = evaluateWithDelegate(new Statement() {
+            @Override
+            public void evaluate() {
+                ThreadGroup group = currentThread().getThreadGroup();
+                assertSame("inner thread should use the same thread group", outerThreadGroup, group);
+            }
+        });
+
+        runnable.run();
+    }
 }