Skip to content
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.

Sign up for GitHub

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

ThreadGroup instance leaked when using Timeout rule #1517

Merged
merged 4 commits into from
Jun 20, 2018
Merged

ThreadGroup instance leaked when using Timeout rule #1517

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
893a880
ThreadGroup instance leaked when using Timeout rule
elrodro83 Apr 25, 2018
de7b482
review
elrodro83 May 9, 2018
f041936
Fix to match JUnit code style
kcooney Jun 3, 2018
0e3fd17
* Reduce join timeout to 1
elrodro83 Jun 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions src/main/java/org/junit/internal/runners/statements/FailOnTimeout.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,22 @@ public void evaluate() throws Throwable {
FutureTask<Throwable> task = new FutureTask<Throwable>(callable);
ThreadGroup threadGroup = new ThreadGroup("FailOnTimeoutGroup");
Thread thread = new Thread(threadGroup, task, "Time-limited test");
thread.setDaemon(true);
thread.start();
callable.awaitStarted();
Throwable throwable = getResult(task, thread);
if (throwable != null) {
throw throwable;
try {
thread.setDaemon(true);
thread.start();
callable.awaitStarted();
Throwable throwable = getResult(task, thread);
if (throwable != null) {
throw throwable;
}
} finally {
thread.join(100);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we can't pass 1 to thread.join()? At this point, the test either completed (in which case this should return right away) or the test timed out (in which case the thread will likely take more than 100ms to exit). Waiting extra time just makes the test run longer.

In fact, maybe we shouldn't call join at all. thread.join() can throw InterruptedException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reduced the timeout as suggested.

join has to be called because otherwise the thread will still be active when calling threadGroup.destroy();, which will throw an IllegalThreadStateException and keep the group referenced, causing the leak.

try {
threadGroup.destroy();
} catch (IllegalThreadStateException e) {
// If a thread from the group is still alive, the ThreadGroup cannot be destroyed.
// Swallow the exception to keep the same behavior prior to this change.
}
}
}

Expand Down
24 changes: 24 additions & 0 deletions src/test/java/org/junit/internal/runners/statements/FailOnTimeoutTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static java.lang.Long.MAX_VALUE;
import static java.lang.Math.atan;
import static java.lang.System.currentTimeMillis;
import static java.lang.Thread.currentThread;
import static java.lang.Thread.sleep;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.hamcrest.core.Is.is;
Expand All @@ -12,6 +13,10 @@
import static org.junit.Assert.fail;
import static org.junit.internal.runners.statements.FailOnTimeout.builder;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.TimeUnit;

import org.junit.Rule;
Expand Down Expand Up @@ -192,4 +197,23 @@ private void notTheRealCauseOfTheTimeout() {
}
}
}

@Test
public void threadGroupNotLeaked() throws Throwable {
Collection<ThreadGroup> groupsBeforeSet = subGroupsOfCurrentThread();

evaluateWithWaitDuration(0);

for (ThreadGroup group: subGroupsOfCurrentThread()) {
if (!groupsBeforeSet.contains(group) && "FailOnTimeoutGroup".equals(group.getName())) {
fail("A 'FailOnTimeoutGroup' thread group remains referenced after the test execution.");
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The readability of the test can be improved by extracting a method subGroupsOfCurrentThread

@Test
public void threadGroupNotLeaked()
        throws Throwable {

    Collection<ThreadGroup> groupsBeforeSet = subGroupsOfCurrentThread();

    evaluateWithWaitDuration(0);

    for (ThreadGroup group: subGroupsOfCurrentThread()) {
        if(!groupsBeforeSet.contains(group) && "FailOnTimeoutGroup".equals(group.getName())) {
            fail("A 'FailOnTimeoutGroup' thread group remains referenced after the test execution.");
        }
    }
}

private Collection<ThreadGroup> subGroupsOfCurrentThread() {
    ThreadGroup[] subGroups = new ThreadGroup[256];
    int numGroups = currentThread().getThreadGroup().enumerate(subGroups);
    return asList(subGroups).subList(0, numGroups);
}


private Collection<ThreadGroup> subGroupsOfCurrentThread() {
ThreadGroup[] subGroups = new ThreadGroup[256];
int numGroups = currentThread().getThreadGroup().enumerate(subGroups);
return Arrays.asList(subGroups).subList(0, numGroups);
}
}