RxJava 2.0.6 : New undeliverable error handling

Created on 16 Feb 2017 · 18Comments · Source: ReactiveX/RxJava

In the release docs of 2.0.6 it is stated that

If an undeliverable exception is an instance/descendant of NullPointerException, IllegalStateException (UndeliverableException and ProtocolViolationException extend this), IllegalArgumentException, CompositeException, MissingBackpressureException or OnErrorNotImplementedException, the UndeliverableException wrapping doesn't happen.

What is the reason for that?
Also I cannot confirm this behaivor. If I understand correctly

RxJavaPlugins.setErrorHandler(throwable -> {
    if (throwable instanceof OnErrorNotImplementedException) {
        throw new RuntimeExecutionException(throwable);
    }
    Timber.e(throwable, "Error handler reported");
});

Single.fromCallable(() -> {
    throw new IllegalArgumentException();
}).subscribe(o -> Timber.d("onSuccess"));

This should not be wrapped and thus not be re-thrown. But it is.

2.x Question

Source

PaulWoitaschek

All 18 comments

Sorry, while reading my post I saw its just stated about UndeliverableException

PaulWoitaschek on 16 Feb 2017

The intent was to have UndeliverableException(IOException) for example but don't "overwrap" an exception type that is typically the result of some app/library bug like NullPointerException.

akarnokd on 16 Feb 2017

So you think it is best practice to re-throw the error if it's an OnErrorNotImplementedException or ProtocolViolationException and log it otherwise? Like:

RxJavaPlugins.setErrorHandler(throwable -> {
    if (throwable instanceof OnErrorNotImplementedException || throwable instanceof ProtocolViolationException) {
        throw new RuntimeExecutionException(throwable);
    }
    Timber.e(throwable, "Error handler reported");
});

If so it might make sense to write that in the wiki as it seems mandatory to add the handler that way if I understand correctly.
In an App I want my errors to make the app crash and everything else not.

PaulWoitaschek on 16 Feb 2017

Don't rethrow and let the app crash. On Android, you can call Thread.currentThread().getUncaughtExceptionHandler().handleException(throwable) and it will crash the app for you.

akarnokd on 16 Feb 2017

👎2

RxJavaPlugins.setErrorHandler(throwable -> {
    if (throwable instanceof OnErrorNotImplementedException || throwable instanceof ProtocolViolationException) {
        Thread currentThread = Thread.currentThread();
        currentThread.getUncaughtExceptionHandler().uncaughtException(currentThread, throwable);
    }
});

So this is equivalent to the RxJava 1 behavior?

PaulWoitaschek on 16 Feb 2017

RxJava 1 by default didn't call uncaughtException and thus never crashed due to undeliverable erros but swallowed it.

akarnokd on 16 Feb 2017

I got burned by this again. I caused a NPE when touching an object in onSuccess and my app did not crash.

How can I handle errors correctly with RxJava?

When I'm just reporting the uncaught exception, my app crashes in circumstances where it should not crash.
When I just swallow everything I'm silently losing errors.
When I catch UndeliverableException my app will still crash when there are undeliverable IllegalArgumentException.

PaulWoitaschek on 21 Mar 2017

Sounds like you have to do more parameter validation. There exist a safeSubscribe() method for Flowable and Observable that tries to save you if your Subscriber or Observer-based implementation throws (which it shouldn't do btw.) The others can't do much about such case because all their methods are terminal state. Therefore, you have to use try-catch in your code to safeguard an unreliable body. If you say that's tedious, introduce another abstract class based on one of the standard abstract consumer type, implement a try-catch in the onXXX methods and have another set of abstract methods to be implemented by actual consumer classes.

akarnokd on 21 Mar 2017

I find it really confusing and got burned again. Maybe you can add an entry in the wiki what a good error handling for android looks like?

For example this:

    Single.timer(2, TimeUnit.SECONDS)
        .subscribe(__ -> {
          throw new RuntimeException();
        });

Why does this get wrapped as an UndeliverableException?

PaulWoitaschek on 27 Mar 2017

The wiki has been updated a couple of days ago.

That is supposed to signal an OnErrorNotImplementedException: https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/Single.java#L2659

akarnokd on 27 Mar 2017

So should I report it as a bug?

Here is a faililng test case:

  @Test
  fun testThrowsOnErrorNotImplemented() {
    RxJavaPlugins.setErrorHandler {
      check(it is OnErrorNotImplementedException)
    }
    Single.just(0).subscribe { _ -> throw RuntimeException() }
  }

(it fails because it's an UndeliverableException

PaulWoitaschek on 27 Mar 2017

No need. I overlooked Single as the base type in your example. A failing lambda can't call onError on Single but only the global error handler.

akarnokd on 27 Mar 2017

And why not? And why can completable and observable do so?

PaulWoitaschek on 27 Mar 2017

CallbackCompletableObserver calls

    @Override
    public void onComplete() {
        try {
            onComplete.run();
        } catch (Throwable ex) {
            Exceptions.throwIfFatal(ex);
            onError(ex);
            return;
        }
        lazySet(DisposableHelper.DISPOSED);
    }

But ConsumerSingleObserver calls

    @Override
    public void onSuccess(T value) {
        try {
            onSuccess.accept(value);
        } catch (Throwable ex) {
            Exceptions.throwIfFatal(ex);
            RxJavaPlugins.onError(ex);
        }
    }

So Completable delegates to onError and Singlet to the plugin? Can't it just call its own onError too?

PaulWoitaschek on 27 Mar 2017

Looks like a bug in the completable handler since that violates the contract.