toIterable() consistent wrapping of errors

[johngmyers]

toIterable() now wraps errors reported through the Observable in a (newly created) ObservedException. This allows a catch block to handle only the exceptions that came through the Observable, not catching exceptions thrown by RxJava itself.

[cloudbees-pull-request-builder]

RxJava-pull-requests #25 FAILURE
Looks like there's a problem with this pull request

[benjchristensen]

What use case are you trying to handle here? I'm not understanding why we would need to wrap everything in an ObservedException - I see it as far better to leave RuntimeExceptions untouched so the try/catch can catch whatever RuntimeException wanted (just propagate) rather than have everything be inside an ObservedException.

This is related to the discussion over on #172 (comment)

[johngmyers]

I added/modified unit tests to describe the use cases.

The problem with leaving RuntimeExceptions untouched is demonstrated in the testToIterableDoesntConfuseExceptionSources test. If you don't always wrap with a specific exception type, then you risk having a catch block intended to handle the Observable's errors unintentionally sweep up exceptions thrown by RxJava. The catch block would be likely to hide the problem, hinder diagnosis, or cause who knows what damage.

The particular exception I used for the unit test is a bit contrived, but I didn't have time to cast about for a plausible input-dependent exception thrown by RxJava.

[benjchristensen]

What confusion are you trying to avoid? It is a truly rare scenario that an exception would be thrown that is not from the Observable sequence (it would mean a very bad bug) or it is something obvious like UnsupportedOperationException that should blow up and make a developer stop using that method.

Am I correct that you would prefer checked Exceptions and are trying to treat RuntimeExceptions emitted from an Observable.onError as if they are checked?

Here is an example of the Rx.Net implementation in C# where the error is thrown: http://rx.codeplex.com/SourceControl/changeset/view/6bf8a7952bdd#Rx/NET/Source/System.Reactive.Linq/Reactive/Linq/Observable/GetEnumerator.cs

        public bool MoveNext()
        {
            _gate.Wait();

            if (_disposed)
                throw new ObjectDisposedException("");

            if (_queue.TryDequeue(out _current))
                return true;

            _error.ThrowIfNotNull();

            Debug.Assert(_done);

            _gate.Release(); // In the (rare) case the user calls MoveNext again we shouldn't block!
            return false;
        }

Note this line: _error.ThrowIfNotNull();

I agree with their implementation (which I didn't look at until now) and it does what RxJava currently does - if an exception is received via onError it propagates it.

Exceptions of any type from any number of sequences or causes can be passed via onError, so capturing and wrapping them will not accomplish any sort of "isolation" from only those causes by a single sequence. Any "RxJava" or other exceptions (JDK, 3rd party libraries, Groovy/Scala/Clojure/JRuby, whatever) of any Observable sequence combined into this iterator() will all be emitted via onError except for the remove() method that throws UnsupportedOperationException. Everything is asynchronous and thus everything is funneled via onError.

If when working on an Observable or iterable from an Observable you know specific exceptions to handle you can, but RuntimeExceptions can and will occur and must either be handled or allowed to propagate.

try {
  it.next();
} catch(AnExceptionBeingLookedFor e) {
  ... do something with e ...
} catch(RuntimeException e) {
  ... generic catch all for anything else ...
}

Or just let it propagate:

try {
  it.next();
} catch(AnExceptionBeingLookedFor e) {
  ... do something with e ...
}

If everything was wrapped virtually all exceptions (including 'framework' ones from the Observable onSubscribe behavior) would all be wrapped and thus handled like this:

try {
  it.next();
} catch(ObservedException e) {
  if(e.getCause() instanceof AnExceptionBeingLookedFor) {
    ... do something with (AnExceptionBeingLookedFor)e ...
  } else {
  ... do something with generic e ...
  }
}

[codefromthecrypt]

Is it fair to summarize the questions are these?

• Should we propagate runtime exceptions (like USOE) in another runtime exception?
• If so, does using a specific subclass of RuntimeException give us more reliable error handling?

I also wonder if there are other sources we can look at for similar wrappiness rationale. (ex. UncheckedExecutionException used many places in guava)

[johngmyers]

If it is considered extremely unlikely that the observable and the framework would send the same typed exception, one possibility would be to wrap only checked exceptions in ObservedException.

"Rare" doesn't quite cut it--many disasters are caused by rare occurrences. I'm not worried about situations, such as the remove() operator's UnsupportedOperationException, that wouldn't escape unit testing, just ones that are input-dependent.

I need to do more research in order to respond to your other points and questions. Unfortunately, I'm severely time-constrained the next few days.

[benjchristensen]

I agree that the 'rare' argument is bad - but that's not the intent of my point - I worded that poorly. My point is that almost all exceptions will be emitted via onError, including "non-business-logic" exceptions.

The onError method is going to receive any mixture of exceptions (Exception or RuntimeException) coming from whatever source occurred (the onSubscribe, Rx itself, the JVM, any libraries involved in it, and any Observables composed into the Observable being subscribed to) thus wrapping the exception being emitted by onError is not going to buy anything - it IS the source of exceptions.

Observables are not just for handling asynchronous values, but asynchronous exceptions as well.

In that diagram note that there are 3 Observable sequences combined into 1 which is then subscribed to via the toIterable() which converts an asynchronous sequence into a synchronous Iterable.

Any of those Observable can emit an Exception/RuntimeException that originates from RxJava itself (a bug), an error from the onSubscribe functions (what is being observed) or anything underneath the onSubscribe (JDK, NPEs, 3rd party libraries).

Since all of the exceptions from A, B, and C are funneled into D they are propagated via onError.

Thus, all of them will be wrapped as ObservedException and nothing treated differently - as there is nothing to distinguish them.

Here is a simple example showing an Observable with try/catch behavior that correctly propagates any errors that occur:

// this is a contrived example that obviously does no real network traffic
Observable<VideoList> getListOfLists(userId) {
    return Observable.create({ observer -> 
        BooleanSubscription subscription = new BooleanSubscription();
        try {
            // this will happen on a separate thread as it requires a network call
            executor.execute({
                    // simulate network latency
                    Thread.sleep(180);
                    for(i in 0..15) {
                        if(subscription.isUnsubscribed()) {
                            break;
                        }
                        try {
                            observer.onNext(new VideoList(i))
                        }catch(Exception e) {
                            observer.onError(e);
                        }
                    }
                    observer.onCompleted();
            })
        }catch(Exception e) {
            observer.onError(e);
        }
        return subscription;
    })
}

Everything once composed under an Observable will emit via onError (that's the point of onError) and not be thrown (because throwing exceptions in an async environments is meaningless) and thus wrapping everything that comes out of onError before throwing from Iterator.next() does not accomplish the stated goal of differentiating between errors "from the Observable" versus "from elsewhere" since everything is funneled via onError (except the top-most methods on the Iterable object itself since that is after escaping the asynchronous Observable).

In the specific case of synchronous "escapes" from Observable such as toIterable().remove() it is the top-most layer above the Observable composition so it could throw UnsupportedOperationException differently since at that point we are synchronous - but if A, B, C, or D observables invoke an iterable.remove() that throws UnsupportedOperationException that will be caught and emitted via onError since all exceptions must be sent via onError to comply with the Observable contract and be properly handled in an async model.

Other than the few scenarios such as UnsupportedOperationException on the top-most Iterable when escaping from the Observable I can't see any other Exceptions that would not route via onError and thus don't see how the goal of differentiating between "Observable exceptions" and "RxJava exceptions" is being accomplished by wrapping them all as ObservedException since both of those will route via onError.

[johngmyers]

Your example:

try {
  it.next();
} catch(AnExceptionBeingLookedFor e) {
  ... do something with e ...
} catch(RuntimeException e) {
  ... generic catch all for anything else ...
}

won't even compile if AnExceptionBeingLookedFor is a checked exception.

[benjchristensen]

I don't understand what you're trying to say with the statement "won't even compile if AnExceptionBeingLookedFor is a checked exception" in context of an Iterator.

A checked exception can't be thrown from an Iterable or Observable, so AnExceptionBeingLookedFor would only be a RuntimeException (http://docs.oracle.com/javase/6/docs/api/java/util/Iterator.html). Any checked exceptions must be wrapped in a RuntimeException and thrown.

Can you elaborate further to help me understand?

Is it just that you are trying to make checked exceptions work with Iterable/Iterator/Observable?

[cloudbees-pull-request-builder]

RxJava-pull-requests #42 SUCCESS
This pull request looks good

[benjchristensen]

John,

If I understand correctly what you're trying to do here it is:

• if a RuntimeException is received then re-throw it
• if a checked Exception is received, wrap it as a RuntimeException called ObservedException instead of RuntimeException (which is what it currently does) and throw it

Is that correct? If so, why? The ObservedException type doesn't communicate anything more than RuntimeException except that it is a checked exception (if they read the javadocs to learn that this is the only reason ObservedException would be thrown) and I don't see how that changes how anyone would interact with this - other than adding another type people would now need to know about.

[johngmyers]

With the statement "won't even compile if AnExceptionBeingLookedFor is a checked exception" I was pointing out that your pattern for handling specific exceptions yet allowing unhandled RuntimeExceptions to propagate doesn't work for handling checked exceptions. If you then devise a corresponding pattern for checked exceptions, you should then discover the need for something equivalent to ObservedException.

The ObservedException communicates that its cause was passed through Observer.onError(). With the current wrapping with a plain RuntimeException, it is difficult, subtle, or even impossible for a catch block to distinguish between a checked exception of a particular type that was passed through Observer.onError() and a (subclass of) RuntimeException, passed through Observer.onError(), that just happens to have a checked exception of that type in its cause.

In an attempt to bisect this discussion, I have reduced the scope of the commit in this pull request to only affect the behavior of checked exceptions. Should we establish the need to change the wrapping behavior of checked exceptions, we can then consider whether RuntimeExceptions need to be similarly wrapped. There are good arguments against wrapping RuntimeExceptions; I need to audit the code to see if it is worth pressing the issue.

As ObservedException is a subclass of RuntimeException, the revised commit doesn't break any existing callers.

[cloudbees-pull-request-builder]

RxJava-pull-requests #46 SUCCESS
This pull request looks good

[johngmyers]

I found other operators that also need to wrap checked exceptions in ObservedException.

I'm now convinced there's no need to wrap RuntimeExceptions in ObservedException.

[benjchristensen]

I still don't understand why checked exceptions need to be wrapped in ObservedException instead of a RuntimeException.

How does the following:

try {
   iterator.next();
} catch(SomeTypedRuntimeException e} {
   ... do something specific to this runtime exception ...
} catch(ObservedException e) {
  if(e.getCause() instanceof ACheckedException) {
    ... do something ...
  }
} catch(RuntimeException e) {
  ... handle all other exceptions ...
}

offer anything more than this:

try {
   iterator.next();
} catch(SomeTypedRuntimeException e} {
   ... do something specific to this runtime exception ...
} catch(RuntimeException e) {
  if(e.getCause() instanceof ACheckedException) {
    ... do something ...
  } else {
  ... handle all other exceptions ...
  }
}

[abersnaze]

Wouldn't if check in the first sample

if(e.getCause() instanceof ACheckedException) {
... do something ...
}
not be necessary because the ObservedException would only be used to wrap checked exceptions.
On Mar 26, 2013, at 9:46 AM, Ben Christensen [email protected] wrote:

I still don't understand why checked exceptions need to be wrapped in ObservedException instead of a RuntimeException.

How does the following:

try {
iterator.next();
} catch(SomeTypedRuntimeException e} {
... do something specific to this runtime exception ...
} catch(ObservedException e) {
if(e.getCause() instanceof ACheckedException) {
... do something ...
}
} catch(RuntimeException e) {
... handle all other exceptions ...
}
offer anything more than this:

try {
iterator.next();
} catch(SomeTypedRuntimeException e} {
... do something specific to this runtime exception ...
} catch(RuntimeException e) {
if(e.getCause() instanceof ACheckedException) {
... do something ...
} else {
... handle all other exceptions ...
}
}
—
Reply to this email directly or view it on GitHub.

[benjchristensen]

There can be any number of checked exceptions in it so you would still have to check the type if you wanted to respond to only a given checked exception.

An Observable can emit via onError any Exception whether it be checked or runtime, thus the only thing ObservedException would tell in this case is that it extends from Exception but not RuntimeException.

[johngmyers]

If the observable did:

ACheckedException cause = ...;
observer.onError(new SomeOtherTypedRuntimeException(cause));

then your first example will work correctly, but your second example will incorrectly unwrap SomeOtherTypedRuntimeException and incorrectly handle cause.

[benjchristensen]

Nested causes will not be solved by any of this. It would never be wrapped in an ObservedException because SomeOtherTypedRuntimeException would be seen as a RuntimeException, not the underlying cause ACheckedException.

We will not unwrap something we receive via onError until we hit a checked Exception (if one even exists) - that is not our call to make as we would then be discarding information that was sent via onError.

Observables emit exceptions - either checked or runtime. It is up to the Observable what they choose to emit.

At the other end of the sequence where we now are breaking out of Observable into an Interable we need to throw whatever is emitted from onError.

The exceptions may be n-levels deep with causes. It may be a RuntimeException with a checked Exception cause with a RuntimeException cause with a Throwable cause.

If the top-most object in the cause-chain is a RuntimeException we just throw it.

If it is a checked Exception then we must wrap as a RuntimeException and throw it.

I still do not know of a use case that wrapping checked exceptions in a special type solves better than just leaving the behavior as is and throwing a RuntimeException.

No matter what is done with the checked Exception someone must query the cause to do anything with it.

If the Observable implementation wrapped a checked Exception in a RuntimeException already that is up to the Observable to do, nothing at all with the toIterable() or exception propagation logic.

[johngmyers]

SomeOtherTypedRuntimeException is not supposed to be wrapped in ObservedException. Please re-read my previous comment.

[johngmyers]

Unlike for RuntimeException, the set of possible checked exception classes is explicitly enumerated by the API definition.

Consider a HystrixCheckedCommand<R, E extends Exception> :

public abstract class HystrixCheckedCommand<R, E extends Exception> {
    protected abstract R run() throws E;

    public R execute() throws E {
        try {
            return new CommandObservable().single();
        } catch (ObservedException e) {
            throw (E) e.getCause();
        }
    }

    private class CommandObservable extends HystrixObservable<R> {
        @Override
        protected Observable<R> create() {
            Observable.create(...wrap run() ...);
        }
    }
}

There is no source of checked exceptions other than run(), so the catch block knows any checked exception is a subclass of E.

[johngmyers]

To recap: you presented two examples, one using ObservedException and one not, stating that you can't see how the first provides useful semantics not provided by the second.

I provided an example where the first example behaves differently than the second. The first example's behavior is useful.

You replied that the first example would not behave like the second, therefore the example is not useful. That argument is a non sequitur.

[benjchristensen]

Am I correct that in short you desire a mechanism to "know" that something emitted from onError and thrown via Iterable was a "checked" Exception at the point it was emitted from onError?

I see how wrapping it as a new type ObservedException could enable this. However it is not required to do so.

If checked Exceptions are important to you and you don't like them being wrapped in a RuntimeException I suggest not using toIterable or other similar blocking calls which do not support checked Exceptions or you could instead use an operator such as Catch (#28) to catch the checked Exception and then wrap it as you wish so that your try/catch block gets exactly what you're looking for.

Or, as you did in the HystrixCheckedCommand wrap a checked Exception as you wish at the source (as you did to wrap it as ObservedException) so that you can then try/catch what you're expecting.

There are many ways within your control of getting the type of Exception you wish without adding a new arbitrary Exception type to RxJava.

I could easily just add this type to the library but then it's one more thing to explain, document, support and for people to understand as it does not obviously fit in. In short, it is not self-explanatory as to why someone would catch it instead of just RuntimeException.

I have tried to understand this conversation and reviewed this issue with several people in person to try and determine what use case is only accomplished by adding this new type and can not come up with a strong reason for adding it.

[benjchristensen]

Closing this out ...