dontCatchUncaughtExceptions by default

[antkorwin]

What do you think about making the dontCatchUncaughtExceptions a setting by default?

I think, sometimes catchs of uncaught exceptions lead to unexpected behaviour in tests,

for example:

@Test
public void testFailed() throws InterruptedException {
    ThreadPoolTaskExecutor taskExecutor = setUpExecutor();
    taskExecutor.execute(new ErrorTestTask());
    ListenableFuture<?> future = taskExecutor.submitListenable(new ErrorTestTask());
    Awaitility.await()
              .atMost(1, TimeUnit.SECONDS)
              .pollInterval(10, TimeUnit.MILLISECONDS)
              .until(future::isDone);
}

@Test
public void testSuccess() throws InterruptedException {
    ThreadPoolTaskExecutor taskExecutor = setUpExecutor();
    taskExecutor.execute(new ErrorTestTask());
    ListenableFuture<?> future = taskExecutor.submitListenable(new ErrorTestTask());
    Thread.sleep(1000);
     Assertions.assertThat(future.isDone()).isTrue();
}

private ThreadPoolTaskExecutor setUpExecutor() {
    ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
    executor.setThreadNamePrefix(THREAD_NAME_PREFIX);
    executor.setMaxPoolSize(1);
    executor.afterPropertiesSet();
    return executor;
}

private static class ErrorTestTask implements Runnable {
    @Override
    public void run() {
        try {
            Thread.sleep(100);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        throw new RuntimeException(Thread.currentThread().getName() + " -> Error");
    }
}

When I replaced Thread.sleep on the Awaitility, I got a failed test.

I encountered this problem when I refactored a lot of tests to use the Awaitility instead of the Thread.sleep. It took quite a long time to understand that the problem was in another thread.

[johanhaleby]

Hi,

Thanks for your comment. You're the first person to say this ever in the Awaitility's 8 year history so I'm a bit hesitant on changing this. Especially since many times (I would guess most of the times?) when testing systems that are using multiple threads you want to get the error regardless of which thread threw the exception. You might not know which thread that is causing the error and you just want to fail your test any error. Also this would break backward compatibility which is also negative.

WDYT?

[antkorwin]

I am confused by the situation, that a test without using the Awaitility will be successful, but it breaks with the Awaitility. Most likely this is more actual for the process of refactoring old tests than for creating a new test.

I completely agree with the fact that breaking back compatibility is bad.

Maybe it will be reasonable to make an example of usage the dontCatchUncaughtExceptions immediately in the documentation of Awaitility. To create a more attention to this point...

[johanhaleby]

I am confused by the situation, that a test without using the Awaitility will be successful, but it breaks with the Awaitility. Most likely this is more actual for the process of refactoring old tests than for creating a new test.

Guess I'm a bit biased since I always start off by using Awaitility ;) Now I better understand what your concern is. I would still suspect that it's more common that you find the current default to be useful otherwise I think someone should have brought this up before.

Maybe it will be reasonable to make an example of usage the dontCatchUncaughtExceptions immediately in the documentation of Awaitility. To create a more attention to this point...

I'm all for improving the docs! If you have an example of what you would like to see I can add it to the docs.

[antkorwin]

I tried to make a commit in the documentation, but I didn't found a way to make a PR in your github wiki.

I put my thoughts here:

---

Exception handling

By default Awaitility catches uncaught throwables in all threads and propagates them to the awaiting thread. This means that your test-case will indicate failure even if it's not the test-thread that threw the uncaught exception.

You can choose to ignore certain exceptions or throwables, see here.

If you don't need to catch exceptions in all threads you can use dontcatchuncaught(link to example 12.).

---

Example 12 - Ignoring uncaught exceptions:

If you migrate your code from using the Thread.sleep to the Awaitility, then in some cases you can get unexpected behavior if your system throws something in other threads, because the Awaitility handles all uncaught exceptions by default. And if you early used Thread.sleep then most likely you didn't catched exceptions from other threads.

You can turn off it by using thedontCatchUncaughtExceptions option:

    @Test
    public void dontCatchUncaughtExample() {
        ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
        executor.setMaxPoolSize(1);
        executor.afterPropertiesSet();

        executor.execute(new ErrorTestTask());
        ListenableFuture<?> future = executor.submitListenable(new ErrorTestTask());

        Awaitility.await()
                  .dontCatchUncaughtExceptions()
                  .atMost(1, TimeUnit.SECONDS)
                  .pollInterval(10, TimeUnit.MILLISECONDS)
                  .until(future::isDone);
    }

    private static class ErrorTestTask implements Runnable {
        @Override
        public void run() {
            try {
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            throw new RuntimeException(Thread.currentThread().getName() + " -> Error");
        }
    }

[johanhaleby]

I made some changes (hope you don't mind) and added it to the docs now. Have a look and tell me what you think.

[antkorwin]

Thanks for your work!
Now this case is clearly described in the documentation.
You can close this issue.

You made a very useful utility, good luck with your projects!

[johanhaleby]

Thank you for the kind words and for the contribution.

[bsideup]

I discovered this issue from @antkorwin's blog post and it was indeed a very surprising behaviour.
I second the opinion that it should not be the default.

P.S. now I'm checking many projects I have (since I love Awailitity and use it in many places) and thinking whether I should explicitly disable this behaviour there, especially when the target system (e.g. a Spring Boot app) runs in the same JVM as the tests' (integration tests, for instance)

[johanhaleby]

@bsideup Thanks for bringing this up again. I'm working on Awaitility 4.0 now so if we want to introduce a breaking change now is a good time.

Maybe you could elaborate to me a bit more why dontCatchUncaughtExceptions should be the default? As I mentioned earlier, I've personally never even once felt that catching uncaught exceptions by default has been a problem. On the contrary, I've actually caught some unexpected bugs due to this behavior. This is of course anecdotal and I don't have any strong feeling or intuitions about which option is the best. However before we change something I want to make sure that I understand the reason(s) :)

From what I can recall from @antkorwin's blog he had tests that used Thread.sleep(..) and changed to Awaitility. While doing this the tests started to fail where they haven't done so previously due to Awaitility's default setting of catching uncaught exceptions. I understand that this might be frustrating, but I'm not yet convinced that this is wrong/bad. I admit that I haven't run the sample test cases that @antkorwin provided here when raising the issue, but when I look at the code in ErrorTestTask I would expect an error to be thrown. Maybe you don't care about this exception, in which case you could ignore it.

Maybe this is a bit naive (sorry), but when I look at the example above I more or less see something similar to this:

public class SomeClass {
    public boolean isDone() {
        try {
            Thread.sleep(100);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        throw new RuntimeException("Error");
    }
}

And if I write a test like this:

SomeClass someClass = new SomeClass();
assertThat(someClass.isDone()).isTrue();

I would except it to fail. So with this reasoning, why wouldn't the same behavior apply if this happens in a different thread.

[johanhaleby]

I have a counter argument to my own example though. Reading the Javadoc of isDone on future suggests that:

Returns true if this task completed. Completion may be due to normal termination, an exception, or cancellation -- in all of these cases, this method will return true.

With this in mind I better understand the argument, at least for Future's. But I'm not sure if it's generalizable? Maybe another option would be to implement something like:

Future f = ..
await().untilDone(f);

where in this case Awaitility wouldn't catch uncaught exceptions (since we're dealing with futures). But maybe this will make things even more complex?

[antkorwin]

I'm working on Awaitility 4.0 now so if we want to introduce a breaking change now is a good time.

Awaitility 4.0 - it's good news!

On the contrary, I've actually caught some unexpected bugs due to this behavior.

Before change something it would be good to consider both sides. Can you show a real example of a test where the default behavior is more expected than "dontCatchUncaughtExceptions"?

I think the main reason for this problem is unexpected behavior for a few cases (migration from Thread.sleep to Awaitility is a real example of this). But if we have only one example of a mismatch between the expected behavior and default behavior, and have a lot of cases where this is more expected then it's quite logical to leave all as it is. Besides, if we change the behavior now, then we break the compatibility, especially if there are many examples of using the default behavior.

Maybe another option would be to implement something like.
Future f = .. await().untilDone(f);

In my opinion, the making of another kind of the asserting for Futures it's not necessary, if you caught this problem and went to documentation then you already found a note about this behavior (and you know how to use uncaught option). I think we have a good chance that a special API for Futures will not be used.

[johanhaleby]

Before change something it would be good to consider both sides. Can you show a real example of a test where the default behavior is more expected than "dontCatchUncaughtExceptions"?

Unfortunately I cannot show an actual example but if I remember it correctly I've been "saved" by it a few times when doing larger integration/acceptance tests. For example if you have a process manager and you kick of a process by sending a command, C, and at the end of the process something should have happened (E). During the life-cycle of this process (C to E) there might be a lot of things happening in different threads and one of these threads might throw an unexpected exception. This has happened to me in the past and this is the reason why "catch all uncaught exceptions" is true by default. I admit that it's happened like 2-3 times at most for me in the 9 year history of Awaitility. You could of course argue that you should have caught these exceptions in other tests, and I agree, but it wasn't until I wrote these larger integration tests where the threading problems became prominent since the entire environment was up and running for a larger use case.

It's hard to know which is the best default option since obviously no one that's positive of having "catch all uncaught exceptions" as default has complained yet :) But I could very well image that your use case might be more frequent, i.e. that you migrate from tests having Thread.sleep(..) to Awaitility and then "catch all uncaught exceptions" by default might be confusing.

WDYT?

[johanhaleby]

@antkorwin @bsideup Any comments? This is the main concern I have before releasing 4.0 and I'd like to proceed :)