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
Declarative timeout does not interrupt code running in infinite loop #2087
Comments
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 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. |
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 |
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 |
@marcphilipp Can you be more specific about the "implement your own |
I meant, instead of using public class PreemptiveTimeoutInvocationInterceptor implements InvocationInterceptor {
@Override
public void interceptTestMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) {
Assertions.assertTimeoutPreemptively(Duration.ofSeconds(1), invocation::proceed);
}
} |
Tentatively slated for 5.6 M2 solely for the purpose of team discussion. |
Team decision: Since a workaround exists, we won't address this for now but wait for additional interest from the community. |
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. |
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 |
Just to be clear: |
So can we have an annotation have the same behavior as |
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 |
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. |
Team decision: Let's add an Enum-valued |
That depends on the definition of "reliably". The 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:
It would be great to be able to:
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 The documentation should probably also mention that |
Here is an example in which
|
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" |
@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? |
@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... |
You'd have to check for the configured config here and probably implement a different version of junit5/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/extension/TimeoutExtension.java Lines 170 to 171 in 17d3b44
|
@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;
} |
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. |
I'm spending some time on this issue. I will create a PR soon. |
Timeouts can now be applied using one of the following three thread modes: `SAME_THREAD`, `SEPARATE_THREAD`, or `INFERRED`. When `SAME_THREAD` is used, the execution of the annotated method proceeds in the main thread of the test. If the timeout is exceeded, the main thread is interrupted from another thread. This is done to ensure interoperability with frameworks such as Spring that make use of mechanisms that are sensitive to the currently running thread — for example, `ThreadLocal` transaction management. On the contrary when `SEPARATE_THREAD` is used, like the `assertTimeoutPreemptively()` assertion, the execution of the annotated method proceeds in a separate thread, this can lead to undesirable side effects, see <<writing-tests-assertions-preemptive-timeouts>>. When `INFERRED` (default) thread mode is used, the thread mode is resolved via the `junit.jupiter.execution.timeout.thread.mode.default` configuration parameter. If the provided configuration parameter is invalid or not present then `SAME_THREAD` is used as fallback. Resolves #2087. Co-authored-by: Marc Philipp <mail@marcphilipp.de>
I think this is not clear and easy for users to understand. If I see It took me more then an hour of google-ing and experimenting, to figure out, why The documentation of I don't have much practice with multithreading, I'm not sure, if I understand correctly, what @marcphilipp means
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? I had a solution written by a student containing an infinite loop, and the Maybe I lack some needed knowledge, but I think that's the point... Users shouldn't have a PhD in multithreading to grook this. This was behaving as expected in jUnit 4 with |
One notable difference in behavior from JUnit 4's |
@blerner I think that's a good idea! Would you mind submitting a PR for that? |
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! |
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:
Context
The text was updated successfully, but these errors were encountered: