2.x: Uncaught errors fail silently in junit tests.

[ZacSweers]

I was writing a utility that involved decorating observers and catching uncaught errors from the delegate observer's onError handling when I noticed in testing that tests always passed. There would be log output, but tests would pass none the less.

Demo of the issue can be found here, but the gist of it is that the following test passes in standard JUnit:

@Test
public void blerg() {
    Observable.error(new RuntimeException("This should fail the test"))
            .subscribe();
}

I'm trying to understand why errors are passed directly to the handler rather than just thrown, and also curious to get thoughts on making this behavior configurable (if for no other reason than to disable in junit tests). I can't find any details about why this behavior happens in JUnit either.

I think this has worrisome implications. In our codebase alone, I dropped in an error watching rule to track these and discovered over 150 tests with uncaught exceptions that are being marked as passing otherwise.

[0kai]

you may use TestObserver to subscribe the result

[akarnokd]

Unlike 1.x, OnErrorNotImplementedException is not thrown. Also please read on the error handling section of the wiki.

[ZacSweers]

I did read that, but it doesn't really explain why it deals directly with the current thread's UncaughtExceptionHandler rather than just throwing if there's no onError hook set.

@0kai I'm not trying to figure out how to test errors, rather I'm trying to make sure that uncaught exceptions in tests aren't marked as passing. At least not silently anyway

[akarnokd]

2.x Subscribers and Observers must not throw from their onXXX methods. Install an error handler before the test and verify there was no undeliverable error. See how we do it in RxJava tests:

RxJava/src/test/java/io/reactivex/parallel/ParallelPeekTest.java

Line 55 in 3bfc275

List<Throwable> errors = TestHelper.trackPluginErrors();

[ZacSweers]

I think I'm not being clear. I'm not trying to test if onXXX methods throw. I'm concerned that errors in the stream with no error handling don't actually fail tests because they're sent directly to the handler rather than just throwing if there's no plugin error handler. Trying to understand why we send directly to the handler.

[akarnokd]

Because you can't throw in a push/async environment and expect it to try-catch it.

[arturdryomov]

We have a very similar issue.

The Thread.UncaughtExceptionHandler documentation states the following.

When a thread is about to terminate due to an uncaught exception the Java Virtual Machine will query the thread for its UncaughtExceptionHandler using getUncaughtExceptionHandler() and will invoke the handler's uncaughtException method, passing the thread and the exception as arguments.

If I understand this correctly, the direct throw will eventually pass the exception to UncaughtExceptionHandler anyway. It seems like the direct pass to UncaughtExceptionHandler without throw will not terminate the current thread. @akarnokd, was this a motivation for not rethrowing the error and using the direct pass to UncaughtExceptionHandler?

Most likely this is not an issue for real-world environments, but this affects unit tests we run on JVM using JUnit.

import org.junit.Test
import io.reactivex.Observable as RxJava2Observable
import rx.Observable as RxJava1Observable

class RxError {

    @Test fun `RxJava 1 test failure with OnErrorNotImplementedException as expected`() {
        RxJava1Observable.fromCallable { throw RuntimeException() }.subscribe()
    }

    @Test fun `RxJava 2 test success, just prints OnErrorNotImplementedException stacktrace not as expected`() {
        RxJava2Observable.fromCallable { throw RuntimeException() }.subscribe()
    }

}

CC @artem-zinnatullin

[akarnokd]

RxJava 2 only passes fatal exceptions upwards, which end up in the uncaught exception handler. Any other exception goes through onError or RxJavaPlugins.onError. RxJavaPlugins.onError() prints the stacktrace and passes the error to the uncaught exception handler of the current thread. This is due to the Reactive-Streams specification which forbids throwing anything non-fatal from Subscribers and by mirror from Observers as well.

RxJava 2 test success, just prints OnErrorNotImplementedException stacktrace not as expected

What stacktrace did you expect?

[arturdryomov]

What stacktrace did you expect?

Sorry for the poor wording. I meant that the expected behaviour from my side is to fail a test due to uncaught exception

[akarnokd]

This is why RxJava's own test often utilize TestHelper.trackPluginErrors() and after the run we check for any errors in the List<Throwable> returned by that method.

[artem-zinnatullin]

Yeah, this is very serious issue for us as well (sad that we started migration that late…)

@akarnokd theoretically what if RxJavaPlugins.onError() would actually throw passed OnErrorNotImplementedException?

For RS subscriber everything will be ok, since OnErrorNotImplementedException is RxJava v2 specific type and StrictSubscriber does not throw it (only if user would do this manually), that would allow RxJava v2 keep compatibility with Reactive Streams and pass TCK.

Basically similar to how it works now with rule 3.9 #5274 and few other cases when if you use RxJava v2 specific types it can break specification, but if you use RS types — it follows RS.

This is kinda breaking change, but not really, since in production ~no one wraps rx chains with try-catch "Because you can't throw in a push/async environment and expect it to try-catch it." and exceptions without try-catch will achieve thread uncaught handler normally as before while in tests users will actually see problems since test frameworks wrap tests with try-catch and reactive chains in tests are mostly synchronous.

Plus we could attach RxJavaPlugin sample that imitates previous behavior to the changelog.

[akarnokd]

The RxJavaPlugins allows you to specify a global handler which you can override temporarily with a @Rule and check for excess errors. The current logic doesn't throw fatal exceptions upwards either so OnErrorNotImplemented wouldn't travel upwards.

[artem-zinnatullin]

Right, but users will have to put @Rule in each test class, not everybody use JUnit (we also use two versions of Spek in our projects and mix JUnit4 with JUnit5), I haven't found a way to set global custom JUnit4 test runner in Gradle and by default IntelliJ runs tests on its own using build system only to generate test class files so it'll also require some customization.

"In production" you typically have an entrypoint (Application classes exist in most of both in front- and backend frameworks) which is a convenient place to hook into RxJavaPlugins and set desired behavior.

While tests are usually isolated from each other and each class if not method will require customization for um, detecting errors… which is basically what each test should do by default… It just seems wrong to not inverse current behavior.

@akarnokd what are your concerns about throwing OnErrorNotImplementedException for RxJava v2 specific subscribers and not doing so for Reactive Streams subscribers?

Mine is that it can create sort of unexpected behavior if you'll try to mix RxJava v2 with some other RS implementations in compare to how RxJava v2 will work in isolation, but I'm not sure that mixing is common use case. I'm much more afraid that current behavior is very unexpected for v1 users who are migrating to v2.