Observer#onError should use Throwable

[Andrei-Pozolotin]

current signature

https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observer.java#L45

uses Exception; instead, lets use Throwable

[benjchristensen]

Throwable only has 2 sub-classes Error and Exception and generally the convention is to not catch Error.

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.
http://docs.oracle.com/javase/6/docs/api/java/lang/Error.html

Thus, all application Exceptions capable of being handled should extend from Exception not Throwable or Error. This was the reasoning behind not using Throwable.

What are the use cases in which we should be catching and passing Error via Observer#onError and thus require Throwable as the signature?

Also of note, this is a breaking change (not a deal breaker since we are pre-1.0, but it's a factor).

[Andrei-Pozolotin]

theoretically speaking, you are right.

one practical use case would be when I want to allow observer
to react to OOME as a way to preserve jvm when "OOME is not really that bad".

there are cases when it is OK for large block heap allocations to fail
with OOME for a given "large" object, which affects only given object execution path
and is not detrimental the rest of the JVM

[benjchristensen]

I have handled that use case before (years ago when working on search engines where it made sense to catch OOME). However, I'd suggest that is an edge case that should be caught by the specific code and turned into an Exception or RuntimeException before being thrown to other code to handle.

Developers should not be expected to handle an Error, including an OOME as they wouldn't know it could be handled.

Here is an example of what I'd suggest to comply with the principle of not throwing Error for developers to handle while still handling the OOME for this edge case.

return Observable.create(new Func1<Observer<String>, Subscription>() {
    def Subscription call(Observer<String> observer) {
        try {
            // allocate memory

            observer.onNext("some value");
            observer.onCompleted();
        } catch(OutOfMemoryError e) {
          observer.onError(new RuntimeException("Recoverable OutOfMemory on optional ... "));
        }

        return Subscriptions.empty();
    };
});

[mttkay]

I agree with the general sentiment that Error isn't something you recover from as part of your business logic, but rather a fundamental flaw in app execution, usually due to developer failure (think divide by zero). That said, I don't think onError should take a Throwable.

One problem we ran into, however, was that throwing Errors such as AssertionError in an observable scheduled on a background thread didn't terminate the app. It would simply continue to execute in what I guess can only be called undefined state. I am not sure yet whether this is something particular about our app or RxJava.

[codefromthecrypt]

My 2p on the issue.

If there's a chance of "user code" being in the stack leading to onError() then we should prefer Throwable as devs can and do raise AssertionErrors intentionally or unintentionally.

Other reasons to prefer Throwable is that the cause of any exception is a throwable. Apis that try to coerce to exception often wrap throwables (sometimes incorrectly) just to handle exception.getCause. This implies that those using or maintaining the system need to carefully review all try-catch blocks before they know that onError will in fact be called and not just propagate stuff and leave them hanging. While there are still cases onError won't be called, due to JVM crash, etc, I think it is still an advantage.

It is possible that Observer writers could be turned off due to throwable. This could be good or bad.. I see this as "hey.. you really should pay attention!!". Those who insist they don't like to receive throwable can punt using or making utilities such as [propagate](http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Throwables.html#propagateIfInstanceOf(java.lang.Throwable, java.lang.Class).

Summary is that if we allow onError to receive a throwable, we are being more honest to the observer about their failures. It will be called more often (for example, it can accept an Error) or would be called with less wrapped things. The try blocks maintaining the stack until onError are simpler and can directly correspond to exception.getCause.

The benefits of onError(Throwable) are worth it.

[codefromthecrypt]

One thing to clarify here is that whether or not to send a throwable signal to the observer is a different issue than whether the code managing the observer propagates errors. If we decide to notify observers when an error (or any other throwable) caused their request to cease, this doesn't imply we are swallowing errors in the RXjava code that calls onError, if that makes sense.

[jmhofer]

I'm strongly against catching Errors. A library shouldn't encourage or even tolerate that its users use Errors in a wrong way (sticking to Oracle's definition here). In all normal use cases, I wouldn't want to handle any errors in onError (which makes the name a bit strange, admittedly). Also, calling onError for an Error and at the same time rethrowing it would be confusing, imho.

[codefromthecrypt]

@jmhofer onError() is basically a message. It doesn't imply the receiver should propagate it. It only informs them what happened, right? Choosing to restrict onError from accepting throwable is a choice to not know why or if your observer was cancelled. I can't side with that.

[codefromthecrypt]

wrt "Also, calling onError for an Error and at the same time rethrowing it would be confusing, imho." It isn't confusing if we understand onError is a signal. Without a callback, the observer could block forever (wait some timeout etc), as they were not informed they will never complete. The Observable can certainly raise an error after signaling its observers. I don't see a disconnect, if we agree on the intent of onError.

[jmhofer]

I agree that raising the error after signaling is better than swallowing it. - Still wouldn't want to handle what basically comes down to a developer's error in onError though.

[codefromthecrypt]

@jmhofer do you mean stick to traditional single-method observer and don't have an onError callback? (hence no discussion about it)

[jmhofer]

@adriancole No, I just think that onError should keep signalling exceptions, not errors, as it does right now.

[codefromthecrypt]

there's several examples of where Throwable is validly sent to callers
(even defined by Oracle [1] and former JCP members [2]). I'll defer to Ben
for the final decision, but I am obviously not in favor of status quo, and
wouldn't want to be a consumer of it with this in mind.

[1]
http://docs.oracle.com/javaee/7/api/javax/websocket/Endpoint.html#onError(javax.websocket.Session,
java.lang.Throwable)
[2]
https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit/RestAdapter.java

} catch (Throwable t) {
throw RetrofitError.unexpectedError(url, t);

[codefromthecrypt]

If we wished to keep the interface, and avoid the "error made me hang
forever" problem, we could update the javadoc and impl such that
onCompleted() will also be called on an Error (admittedly a confusing
compromise).

[codefromthecrypt]

Possibly dead horse, at this point, but every current java async web callback chose throwable, not exception, for error signals. If we chose to retain Exception as opposed to Throwable, converting to or from these interfaces will be lossy.

• Servlet 3.0 AsyncListener
• JAX-RS 2.0 InvocationCallback
• Websocket Endpoint

[benjchristensen]

Those are some very strong arguments and I'm mostly convinced that it's worth changing to Throwable. I still don't think Errors should be handled (most of the time) but I agree that wrapping an Observable around something that throws a Throwable is very awkward if onError only supports Exception as that means contrived Exceptions wrapping `Throwable' or they are thrown from somewhere else and could leave the Observable/Observer permanently blocked waiting on a terminal state.

To adopt onError(Throwable) means a breaking change, but version 0.10.0 is a good time for this since we're changing the builds significantly and requiring people to change their build target from rxjava-core to rxjava as part of it, and that's why we're still not at version 1.x.

I don't like however that onErrorResumeNext will now start handling Throwable and Error, not just Exception.

Perhaps we should create a new onExceptionResumeNext that should be what people normally use? I'd love to make onErrorResumeNext be that way, but the naming is wrong, so onErrorResumeNext needs to receive all errors passed by onError which means Throwable whereas onExceptionResumeNext could catch only Exception and allow Throwable/Error past.

What other implications are there if we proceed with this change?

[benjchristensen]

Anyone have further thoughts on this issue, particularly the use cases I explained in my last comment regarding onErrorResumeNext and onExceptionResumeNext?

[Andrei-Pozolotin]

sounds like a good idea: using only one: onExceptionResumeNext

[benjchristensen]

@Andrei-Pozolotin @adriancole @jmhofer @mttkay @michaeldejong @mairbek

I have submitted a pull request #315 with the breaking change of onError(Exception) to onError(Throwable) based on the discussion in this issue. I would appreciate your review and confirmation before I proceed.

I would like to merge and release within 24 hours if possible.