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.

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

Completely custom timeout approach as suggested by dsaff in #727 #797

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

import java.lang.management.ManagementFactory;
import java.lang.management.ThreadMXBean;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
Expand Down Expand Up @@ -66,30 +67,48 @@ private Throwable getResult(FutureTask<Throwable> task, Thread thread) {
}
}

private Exception createTimeoutException(Thread thread) {
protected Throwable createTimeoutException(Thread thread) {
List<Throwable> exceptions = new ArrayList<Throwable>();

exceptions.add(createCurrentThreadException(thread));
exceptions.addAll(createAdditionalTimeoutExceptions(thread));

return exceptions.size() == 1 ? exceptions.get(0) : new MultipleFailureException(exceptions);
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to MultipleFailureException.assertEmpty, for what it's worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I miss the point: but can you have a second look at that?

My code change above leads to the same behaviour as before my change: which was to collect and return the timeout exception(s): either the single exception or the multiplelfailureexception in case of more than one.

And therefore I don't know where you would like to use MultipleFailureException.assertEmpty

}

private Throwable createCurrentThreadException(Thread thread) {
StackTraceElement[] stackTrace = thread.getStackTrace();
final Thread stuckThread = fLookForStuckThread ? getStuckThread(thread) : null;
Exception currThreadException = new TestTimedOutException(fTimeout, fTimeUnit);
if (stackTrace != null) {
currThreadException.setStackTrace(stackTrace);
thread.interrupt();
}
return currThreadException;
}

/**
* Returns list of optional exceptions with additional timeout failure context like stuck
* thread (if there is one detected and detection is enabled); can be overridden to add custom
* timeout failure context.
*/
protected List<Throwable> createAdditionalTimeoutExceptions(Thread thread) {
List<Throwable> exceptions = new ArrayList<Throwable>();

final Thread stuckThread = fLookForStuckThread ? getStuckThread(thread) : null;
if (stuckThread != null) {
Exception stuckThreadException =
new Exception ("Appears to be stuck in thread " +
stuckThread.getName());
Exception stuckThreadException =
new Exception ("Appears to be stuck in thread " +
stuckThread.getName());
stuckThreadException.setStackTrace(getStackTrace(stuckThread));
return new MultipleFailureException
(Arrays.<Throwable>asList(currThreadException, stuckThreadException));
} else {
return currThreadException;
exceptions.add(stuckThreadException);
}
return exceptions;
}

/**
* Retrieves the stack trace for a given thread.
* @param thread The thread whose stack is to be retrieved.
* @return The stack trace; returns a zero-length array if the thread has
* @return The stack trace; returns a zero-length array if the thread has
* terminated or the stack cannot be retrieved for some other reason.
*/
private StackTraceElement[] getStackTrace(Thread thread) {
Expand All @@ -107,18 +126,18 @@ private StackTraceElement[] getStackTrace(Thread thread) {
* @param mainThread The main thread created by {@code evaluate()}
* @return The thread which appears to be causing the problem, if different from
* {@code mainThread}, or {@code null} if the main thread appears to be the
* problem or if the thread cannot be determined. The return value is never equal
* problem or if the thread cannot be determined. The return value is never equal
* to {@code mainThread}.
*/
private Thread getStuckThread (Thread mainThread) {
if (fThreadGroup == null)
if (fThreadGroup == null)
return null;
Thread[] threadsInGroup = getThreadArray(fThreadGroup);
if (threadsInGroup == null)
if (threadsInGroup == null)
return null;

// Now that we have all the threads in the test's thread group: Assume that
// any thread we're "stuck" in is RUNNABLE. Look for all RUNNABLE threads.
// any thread we're "stuck" in is RUNNABLE. Look for all RUNNABLE threads.
// If just one, we return that (unless it equals threadMain). If there's more
// than one, pick the one that's using the most CPU time, if this feature is
// supported.
Expand All @@ -131,13 +150,13 @@ private Thread getStuckThread (Thread mainThread) {
stuckThread = thread;
maxCpuTime = threadCpuTime;
}
}
}
}
return (stuckThread == mainThread) ? null : stuckThread;
}

/**
* Returns all active threads belonging to a thread group.
* Returns all active threads belonging to a thread group.
* @param group The thread group.
* @return The active threads in the thread group. The result should be a
* complete list of the active threads at some point in time. Returns {@code null}
Expand All @@ -158,16 +177,16 @@ private Thread[] getThreadArray(ThreadGroup group) {
// is >= the array's length; therefore we can't trust that it returned all
// the threads. Try again.
enumSize += 100;
if (++loopCount >= 5)
if (++loopCount >= 5)
return null;
// threads are proliferating too fast for us. Bail before we get into
// threads are proliferating too fast for us. Bail before we get into
// trouble.
}
return copyThreads(threads, enumCount);
}

/**
* Returns an array of the first {@code count} Threads in {@code threads}.
* Returns an array of the first {@code count} Threads in {@code threads}.
* (Use instead of Arrays.copyOf to maintain compatibility with Java 1.5.)
* @param threads The source array.
* @param count The maximum length of the result array.
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/org/junit/rules/Timeout.java
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,18 @@ public static Timeout seconds(long seconds) {
return new Timeout(seconds, TimeUnit.SECONDS);
}

protected final long getTimeout() {
return fTimeout;
}

protected final TimeUnit getTimeUnit() {
return fTimeUnit;
}

protected final boolean isLookForStuckThread() {
return fLookForStuckThread;
}

/**
* Specifies whether to look for a stuck thread. If a timeout occurs and this
* feature is enabled, the test will look for a thread that appears to be stuck
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/junit/tests/AllTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import org.junit.tests.running.core.JUnitCoreReturnsCorrectExitCodeTest;
import org.junit.tests.running.core.SystemExitTest;
import org.junit.tests.running.methods.AnnotationTest;
import org.junit.tests.running.methods.CustomTimeoutTest;
import org.junit.tests.running.methods.ExpectedTest;
import org.junit.tests.running.methods.InheritedTestTest;
import org.junit.tests.running.methods.ParameterizedTestMethodTest;
Expand Down Expand Up @@ -130,6 +131,7 @@
TestMethodTest.class,
TextListenerTest.class,
TimeoutTest.class,
CustomTimeoutTest.class,
EnclosedTest.class,
ParameterizedTestMethodTest.class,
InitializationErrorForwardCompatibilityTest.class,
Expand Down
144 changes: 144 additions & 0 deletions src/test/java/org/junit/tests/running/methods/CustomTimeoutTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
package org.junit.tests.running.methods;

import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;

import java.io.PrintWriter;
import java.io.StringWriter;
import java.io.Writer;
import java.util.List;
import java.util.concurrent.TimeUnit;

import org.junit.Rule;
import org.junit.Test;
import org.junit.internal.runners.statements.FailOnTimeout;
import org.junit.rules.TestRule;
import org.junit.rules.Timeout;
import org.junit.runner.Description;
import org.junit.runner.JUnitCore;
import org.junit.runner.Result;
import org.junit.runners.model.Statement;
import org.junit.tests.running.methods.TimeoutTest.InfiniteLoopMultithreaded;

public class CustomTimeoutTest {
Copy link
Member

Choose a reason for hiding this comment

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

This test seems unrelated to the change. I understand that you want to preserve your original code somewhere, but I'm not sure if we should be maintaining that in a test in the JUnit code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you refer to the CustomTimeoutHandler implementatio of getFullThreadDump() you are right, that is "only" there to (a) show the initial/original motivation of this pull request, and (b) hopefully will be integrated into the production code via some other pull requests (#768).

However, the CustomTimeoutTest as such is of course needed to (a) show the need for changes, (b) self test these changes and (c) give an example of how to use this new "freedom", i.e. the approach as such suggested by dsaff.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think CustomTimeoutTest is needed. There's no code changes in this CL that need to be tested.

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 dare to make one last attempt to change your mind, before I would actually delete CustomTimeoutTest as you are suggesting:

  • Without the test we would have missed some of the required changes (cf. conversations and original change suggestion by dsaff: "(3) [...] visibility of fields").
  • Without this test the code in CustomFailOnTimeout.createTimeoutException() would not have been become like this: and somehow this does not look very nice to me yet or in other words smelly, so maybe the FailOnTimeout classes code with respect to timeout exception creation should be refactored too to allow for nicer re-use in CustomFailOnTimeout. Frankly I have not yet thought about the details or a concrete approach, but only the currently needed code in this test pointed out this kind of weakness? => should I try to elaborate in this direction?
  • I personally think that tests are often the best type of documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Without the test we would have missed some of the required changes

@reinholdfuereder Then I think we should write tests to verify those changes. In other words, have the tests verify the contract that your subclass is expecting.

I personally think that tests are often the best type of documentation.

I agree, but we need to make sure that the tests are maintainable, and that it's clear what's being tested (so if a refactoring breaks a test, we can tell if the problem is with the refactoring or with the test). I like tests, too. I think what you have here is bigger than it needs to be, that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcooney I hope that the recent change to shorten the test and getting rid of the whole full thread dump creation + logging is sufficient. (Please note I am not fully happy about CustomFailOnTimeout.createTimeoutException() yet: while I have full flexibility in my custom timeout exception creation, it feels a bit awkward.)


public static class CustomTimeoutHandler {

public Exception handleTimeout(Thread thread) {
String prefix = "[" + getClass().getSimpleName() + "] ";
return new Exception(prefix + "Appears to be stuck due to running into timeout. Here could be some more custom failure context...");
}

}


public static class CustomFailOnTimeout extends FailOnTimeout {

private CustomTimeoutHandler handler = new CustomTimeoutHandler();

public CustomFailOnTimeout(Statement base, long timeout,
TimeUnit unit, boolean lookForStuckThread) {
super(base, timeout, unit, lookForStuckThread);
}

@Override
protected List<Throwable> createAdditionalTimeoutExceptions(
Thread thread) {
List<Throwable> exceptions = super.createAdditionalTimeoutExceptions(thread);
Throwable handleTimeout = handler.handleTimeout(thread);
exceptions.add(handleTimeout);
return exceptions;
}

}

public static class CustomTimeout extends Timeout {

public CustomTimeout(int timeout, TimeUnit unit) {
this(timeout, unit, false);
}

public CustomTimeout(int timeout, TimeUnit unit, boolean lookForStuckThread) {
super (new Timeout(timeout, unit), lookForStuckThread);
}

@Override
public Statement apply(Statement base, Description description) {
return new CustomFailOnTimeout(base, getTimeout(), getTimeUnit(), isLookForStuckThread());
}
}


public static class InfiniteLoopTest {

@Rule
public TestRule globalTimeout = new CustomTimeout(100, TimeUnit.MILLISECONDS);

@Test
public void failure() {
infiniteLoop();
}

private void infiniteLoop() {
for (; ; ) {
try {
Thread.sleep(10);
} catch (InterruptedException e) {
}
}
}
}

@Test
public void infiniteLoop() throws Exception {
JUnitCore core = new JUnitCore();
Result result = core.run(InfiniteLoopTest.class);
assertEquals(1, result.getRunCount());
assertEquals(2, result.getFailureCount());
Throwable exception[] = new Throwable[2];
for (int i = 0; i < 2; i++)
exception[i] = result.getFailures().get(i).getException();
assertThat(exception[0].getMessage(), containsString("test timed out after 100 milliseconds"));
assertThat(exception[1].getMessage(), containsString("[CustomTimeoutHandler] Appears to be stuck due to running into timeout. Here could be some more custom failure context..."));
}


// --------------------------------------------------------------------------------------------------
// Below is the additional code for test scenario when the timeout is also looking for stuck threads:

public static class InfiniteLoopWithLookForStuckThreadTest {

@Rule
public TestRule globalTimeout = new CustomTimeout(100, TimeUnit.MILLISECONDS, true);

@Test
public void failure() throws Exception {
(new InfiniteLoopMultithreaded()).failure(false);
}

}

@Test
public void infiniteLoopWithLookForStuckThread() throws Exception {
JUnitCore core = new JUnitCore();
Result result = core.run(InfiniteLoopWithLookForStuckThreadTest.class);
assertEquals(1, result.getRunCount());
assertEquals(3, result.getFailureCount());
Throwable exception[] = new Throwable[3];
for (int i = 0; i < 3; i++)
exception[i] = result.getFailures().get(i).getException();
assertThat(exception[0].getMessage(), containsString("test timed out after 100 milliseconds"));
assertThat(stackForException(exception[0]), containsString("Thread.join"));
assertThat(exception[1].getMessage(), containsString("Appears to be stuck in thread timeout-thr2"));
assertThat(exception[2].getMessage(), containsString("[CustomTimeoutHandler] Appears to be stuck due to running into timeout. Here could be some more custom failure context..."));
}

private String stackForException(Throwable exception) {
Writer buffer = new StringWriter();
PrintWriter writer = new PrintWriter(buffer);
exception.printStackTrace(writer);
return buffer.toString();
}

}