Declarative timeout does not interrupt code running in infinite loop

[praenubilus]

Steps to reproduce

Having some code running infinitely, but the timeout annotation(neither the class level nor testable method level) can interrupt the execution. Instead, the program hang forever.

I was hoping it can behave like the assertTimeoutPreemptively to interrupt the running test method.

e.g. I have a class with just one single infinite running method:

public class A {

    public void infinite() {
        while (true) {
            continue;
        }
    }
}

    @Test
    @Timeout(1) // run at most 1 seconds
    void pollUntil() {
        A a = new A();
        a.infinite();
        assertEquals(1,1);
    }

Context

• Used versions: Jupiter 5.5.2

[marcphilipp]

assertTimeoutPreemptively only seems to work but actually hides the underlying problem that infinite() never checks for interrupts or calls any blocking operation that would throw an InterruptedException. So, instead of blocking indefinitely, assertTimeoutPreemptively lets the thread it spawned run forever.

@Timeout on the other hand works differently. It schedules a thread to interrupt the original thread once the timeout has expired. If the original thread does not react, it will keep running until it's eventually finished, or, as in this case, until you reboot your computer. 😉

The underlying problem is that there's no API in Java that let's you reliably terminate a thread. Thus, I don't see what we could change to support methods like infinite().

Personally, I think blocking indefinitely is better than hiding the problem by keeping the code running in the background. The latter might cause strange side effects if it changes any shared state and might even lead to the JVM running out of threads when there's many such tests.

[praenubilus]

Hell @marcphilipp ,

Thanks for the feedback. I remember someone brought up this before, when the Timeout annotation being proposed: we have lots of legacy testing code in JUnit 4 relies on the timeout rules, which works in a preemptive way (from my own observation). I totally agree with you the behavior will hide some problems under the hood. But in some scenarios, the preemptive behavior is really helpful. For example we have an online Java education service, the students' submissions are totally unpredictable and we evaluate/grades the code heavily relying on the number of pass/fail unit tests. If there is some infinite running part in the code for one test case, we have no way to give a grade because the service is keep running forever. But that can be terminated in our JUnit 4 implementation with a timeout rule configuration. As we are migrating the system to JUnit 5, this becomes a headache for us. The current work around we do is enclosing every single unit test with the assertTimeoutPreemptively. And for the potential threads running in the background issue, because we run the services in a docker container, as long as all unit tests have a pass/fail result, the container will be shutdown. So that will not be an issue.

I know the aforementioned scenario is a little bit special, but I do see someone else also have the similar requirements in their code base as we use JUNIT as a part of the business logic. The new Timeout annotation introduced in v5.5 is elegant and convenient to use except lacking the preemptive behavior we want. The workaround I am thinking about: is it possible to add some "switcher" to the annotation which may look like following:

        @Test
    @Timeout(1, PREEMPTIVE=true) // run at most 1 seconds
    void pollUntil() {
        A a = new A();
        a.infinite();
        assertEquals(1,1);
    }

Or just introducing a new annotation like @TimeoutPreemptive?

[marcphilipp]

How are you executing the tests? Maybe there's a way to add a timeout around all tests? As a workaround, you could implement your own InvocationInterceptor that wraps calls to invocation.proceed() in assertTimeoutPreemptively.

[praenubilus]

How are you executing the tests? Maybe there's a way to add a timeout around all tests? As a workaround, you could implement your own InvocationInterceptor that wraps calls to invocation.proceed() in assertTimeoutPreemptively.

@marcphilipp In JUnit4 we write a program to invoke all tests by using JUnitCore.runClasses(TestClass) to start all tests, get and analyze the results. In JUnit 5 we use org.junit.platform.launcher.core.LauncherFactory.

Can you be more specific about the "implement your own InvocationInterceptor"? I partially get what you mean, but I believe a genuine official solution will always be the best choice.

[marcphilipp]

I meant, instead of using @Timeout you could register your own extension that does this:

public class PreemptiveTimeoutInvocationInterceptor implements InvocationInterceptor {
    @Override
    public void interceptTestMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) {
        Assertions.assertTimeoutPreemptively(Duration.ofSeconds(1), invocation::proceed);
    }
}

[marcphilipp]

Tentatively slated for 5.6 M2 solely for the purpose of team discussion.

[marcphilipp]

Team decision: Since a workaround exists, we won't address this for now but wait for additional interest from the community.

[kreimendahlf]

I am caught with the same use-case as @praenubilus : migrating grading scripts from JUnit 4 -> 5, and running into infinite loops that aren't timed out.

I would also be interested in a preemptive timeout that matches the language for non-preemptive.

[codejayant]

I am working on a similar use case where I would like to run a condition infinitely until the right condition matches or timeout occur which should stop the execution. Will be good if there is an option to stop the execution on timeout in @Timeout annotation.

[marcphilipp]

Just to be clear: @Timeout is preemptive. It interrupts the thread that's running the test when it times out. However, if that thread does not react to being interrupted, there's no way to stop it. assertTimeoutPreemptively does the same thing, however, it executes the supplied Executable in a different thread. If that thread does not respond to interrupts, it will continue to run, even after assertTimeoutPreemptively returns.

[praenubilus]

Just to be clear: @Timeout is preemptive. It interrupts the thread that's running the test when it times out. However, if that thread does not react to being interrupted, there's no way to stop it. assertTimeoutPreemptively does the same thing, however, it executes the supplied Executable in a different thread. If that thread does not respond to interrupts, it will continue to run, even after assertTimeoutPreemptively returns.

So can we have an annotation have the same behavior as assertTimeoutPreemptively ? Actually that's what I proposed in October last year, but the team decides not to include it at that time.

[marcphilipp]

I'd be okay with making this configurable as long as the current behavior stays the default.

[praenubilus]

I'd be okay with making this configurable as long as the current behavior stays the default.

Great. This is a long time expected feature so we can finally align and migrate all JUnit 4 test cases. Will it look like any one of following annotations?

@Timeout(1, PREEMPTIVE=true) // default is false 

@Timeout(1, FORCEQUIT=true) // default is false

@Timeout(1, UNSAFE=true)  // default is false

@TimeoutPreemptive(1)  // behave like assertTimeoutPreemptively

@TimeoutForceQuit(1)  // behave like assertTimeoutPreemptively

@TimeoutUnsafe(1)  // behave like assertTimeoutPreemptively

[marcphilipp]

Naming things is always hard. I was thinking about naming it more along the lines as to in which thread the test is going to be executed. That would yield sth. like "same thread" for the current behavior and "separate thread" for the new behavior. But I'm not really happy with those either.

@junit-team/junit-lambda Any ideas?

We also need a configuration parameter as a default timeout can be configured via configuration parameters as well. Maybe it would even be easier to start with just that and not add new annotation or attribute.

[marcphilipp]

Team decision: Let's add an Enum-valued mode attribute to @Timeout with values INFERRED (default), SAME_THREAD, and SEPARATE_THREAD along with a junit.jupiter.execution.timeout.mode.default configuration parameter to configure the default.

[christianhujer]

The underlying problem is that there's no API in Java that let's you reliably terminate a thread.

That depends on the definition of "reliably". The Thread.stop() is reliable in the sense that it will reliably stop a Thread. It's also "unsafe" as this may leave objects in an inconsistent state, which is the reason why it is deprecated (but not scheduled for removal). In the context of having a reliable timeout for tests, this may be exactly what some users (including me) might want.

https://docs.oracle.com/en/java/javase/15/docs/api/java.base/java/lang/Thread.html#stop()

Here's my test problem, simplified for reproduction:

import java.net.ServerSocket
import java.time.Duration

class SomeTest {
    @Timeout(1)
    @Test
    fun testSleep() {
        // works as expected.
        Thread.sleep(2000)
    }

    @Timeout(1)
    @Test
    fun testAccept() {
        // never stops
        ServerSocket(0).accept()
    }

    @Test
    fun testAccept2() {
        // times out as expected.
        // However, the code that called ServerSocket(0).accept() remains active and blocked.
        // This may be acceptable in many cases.
        assertTimeoutPreemptively(Duration.ofSeconds(1)) {
            ServerSocket(0).accept()
        }
}

It would be great to be able to:

• Have a declarative way of preemptive timeouts: @Timeout(mode=PREEMPT) should declaratively lead to the same behavior as assertTimeoutPreemptively().
• Have a way to use Thread.stop() for stopping the execution: @Timeout(mode=UNSAFE).

Note: I have not done any comparison with JUnit 4. I'm just adding my 2 cents, as I have run in the same issue today that @Timeout() only works on code that reacts to Thread.interrupt(). It would be very helpful, as long as there is no implementation of other declarative timeouts, that the API documentation of @Timeout mentions that this currently works only for code that reacts to Thread.interrupt().

The documentation should probably also mention that UNSAFE timeouts should be avoided. Preemptive timeouts and proper cleanup code should in most cases be completely sufficient, and will automatically lead to better and cleaner production code designs.

[dkandalov]

Here is an example in which @Timeout does fail (on a "performance bug") but only after 30 seconds instead of 1 second as expected:

class Tests {
    @Timeout(1)
    @Test fun `large input`() {
        val random = Random(seed = 42)
        val ints = generateSequence { random.nextInt(-105, 106) }.take(3000).toList()

        val triplets = ints.findZeroSumTriplets()
        assertThat(triplets.size, equalTo(303))
    }
}

fun List<Int>.findZeroSumTriplets(): Set<Triplet> {
    val result = HashSet<Triplet>()
    0.rangeTo(lastIndex - 2).forEach { i1 ->
        (i1 + 1).rangeTo(lastIndex - 1).forEach { i2 ->
            (i2 + 1).rangeTo(lastIndex).forEach { i3 ->
                if (this[i1] + this[i2] + this[i3] == 0) {
                    result.add(Triplet(this[i1], this[i2], this[i3]))
                }
            }
        }
    }
    return result
}

data class Triplet(val value: List<Int>) {
    constructor(first: Int, second: Int, third: Int): this(listOf(first, second, third).sorted())
    init {
        require(value.size == 3 && value == value.sorted())
    }
}

[yoshivda]

Is there any update on this? I'm still running into the same problems as described above. We use tests to grade student submissions and after migrating from JUnit 4 to 5 we run into execution times going over the limit of our system, thereby giving a generic timeout warning without any test results instead of simply saying "test x timed out"

[marcphilipp]

@yoshivda There's no update since the team hasn't had time to work on it, yet. Would you be interested in submitting a PR?

[yoshivda]

@marcphilipp I am in no way familiar with the JUnit source code and this change seems to be quite involved. Adding the Enum to the Timeout class is not that big of a deal, but I wouldn't know where to go next...

[marcphilipp]

You'd have to check for the configured config here and probably implement a different version of TimeoutInvocation that runs the timeout in the executor service.

https://github.com/junit-team/junit5/blob/17d3b440f3b6c3ba4616fc927e7782e74582ea86/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java#L170-L171

[gitorko]

I meant, instead of using @Timeout you could register your own extension that does this:

public class PreemptiveTimeoutInvocationInterceptor implements InvocationInterceptor {
    @Override
    public void interceptTestMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) {
        Assertions.assertTimeoutPreemptively(Duration.ofSeconds(1), invocation::proceed);
    }
}

@Test
    public void test_timeout() {
        Assertions.assertTimeoutPreemptively(Duration.ofSeconds(1), () -> {
            Assertions.assertEquals(4, square(2));
        });
    }

    @Test
    public void test_withoutTimeout() {
        Assertions.assertTimeoutPreemptively(Duration.ofSeconds(1), () -> {
            Assertions.assertEquals(9, square(3));
        });
    }

    public int square(int x) {
        if (x == 2) {
            while (true) ;
        }
        return x * x;
    }

[Cody-Banks7]

I'm wondering if this problem is still worth trying? I'd like to try to handle it but find it hard to start with. Some guidance would be very helpful.

[gilbertojrequena]

I'm spending some time on this issue. I will create a PR soon.

[adamsan]

I think this is not clear and easy for users to understand.

If I see @Timeout(24) on a test method, I expect it to fail, if it executes longer than 24 seconds. It shouldn't matter, if it handles interrupts or not...

It took me more then an hour of google-ing and experimenting, to figure out, why @Timeout doesn't work, (or behave, as expected) - it didn't stopped code containing an infinite loop. Turns out what I needed is: @Timeout(value = 24, threadMode = Timeout.ThreadMode.SEPARATE_THREAD)

The documentation of Timeout doesn't mention this. At least the javadoc for Timeout should be clearer.

I don't have much practice with multithreading, I'm not sure, if I understand correctly, what @marcphilipp means

assertTimeoutPreemptively only seems to work but actually hides the underlying problem that infinite() never checks for interrupts or calls any blocking operation that would throw an InterruptedException. So, instead of blocking indefinitely, assertTimeoutPreemptively lets the thread it spawned run forever.

Does it mean, when while the other test methods are running after the infinite looping test, the thread running the infinite loop still continues to execute, until the tests are done and the JVM stops? As it currently works, it runs forever, unless I kill the process... How is this not worse?

I had a solution written by a student containing an infinite loop, and the mvn test in our Github Action was running for 4 hours before I luckily noticed, and killed manually.

Maybe I lack some needed knowledge, but I think that's the point... Users shouldn't have a PhD in multithreading to grook this. Using a timeout for a test shouldn't be this WTF moment.

This was behaving as expected in jUnit 4 with @Test(timeout=...).

[blerner]

One notable difference in behavior from JUnit 4's FailOnTimeout vs JUnit 5's AssertTimeoutPreemptively implementations is that FailOnTimeout used setDaemon to let threads running infinite() to not block the shutdown of the JVM when the rest of the testing process is complete. It seems to me that a tiny change to TimeoutThreadFactory that makes the returned thread setDaemon(true) would make JUnit 5's behavior here match JUnit 4's, without compromising any of the other constraints JUnit 5 operates on. I tried this in my own code, and it seems to work great.

[marcphilipp]

@blerner I think that's a good idea! Would you mind submitting a PR for that?

[blerner]

Glad it seems like a good idea! I'm not currently set up to build JUnit itself, nor am I entirely confident at the moment how I'd write a regression test to confirm that these daemonized threads are working as intended. (I'm very new to JUnit 5;s codebase, and haven't at all looked at junit's own test suite.) So I probably couldn't contribute a high-quality PR right now.

After eliminating all my copy-paste-modifed code from my own project, the effective diff here is just

$ diff AssertTimeoutPreemptively.java AssertTimeoutPreemptively-daemon.java 
153c153,155
<           return new Thread(r, "junit-timeout-thread-" + threadNumber.getAndIncrement());
---
>           Thread timeoutThread = new Thread(r, "junit-timeout-thread-" + threadNumber.getAndIncrement());
>           timeoutThread.setDaemon(true);
>           return timeoutThread;

If anyone wants to turn that code into a PR, fine by me!