RxJava 2.0.6 : New undeliverable error handling

[PaulWoitaschek]

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.

[PaulWoitaschek]

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

[akarnokd]

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.

[PaulWoitaschek]

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.

[akarnokd]

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.

[PaulWoitaschek]

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?

[akarnokd]

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

[PaulWoitaschek]

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.

[akarnokd]

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.

[PaulWoitaschek]

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?

[akarnokd]

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

[PaulWoitaschek]

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

[akarnokd]

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.

[PaulWoitaschek]

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

[PaulWoitaschek]

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?

[JakeWharton]

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

[akarnokd]

Indeed, that looks like a copy-paste error.

[akarnokd]

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

Observable has onNext which when using lambdas may end up in onError() legally.

[akarnokd]

Looks like this question has been answered. If you have further input on the issue, don't hesitate to reopen this issue or post a new one.