Error Handling: OnErrorNotImplemented and java.lang.Error

[benjchristensen]

Special Handling of java.lang.Error and OnErrorNotImplemented

• StackOverflowError is swallowed  #748 (comment)
• Exception rethrown inconsistently with/without .flatMap() in 0.16.1 #771
• Exception in subscribe's onNext is not rethrown #789

[benjchristensen]

This represents a possible approach to handling OnErrorNotImplementedException and java.lang.Error. I'm not convinced it's the right approach for all java.lang.Error to be caught.

For OnErrorNotImplementedException and StackOverflow I think they must be thrown from wherever they are caught.

The reason for it not being obvious to throw is that by throwing on a background thread instead of sending via onError the terminal states of an Observable will never be triggered and thus will deadlock. Many java.lang.Error should result in the system shutting down, but probably not all. It could possibly be limited to these: #748 (comment)

Any advice or insight on what is considered to be the right approach for everything else?

[benjchristensen]

After thinking about it further I decided I didn't like my previous approach and have replaced it with this one that is more generic and doesn't make any specific operators do anything special.

I found the issue was the SynchronizedObserver was taking on more responsibility than it should have been and was capturing errors and then ignoring everything after a terminal state occurred which became a problem when onError was called, resulted in an exception of its own which invoked onError again.

This resulted in me doing 3 specific changes:

1. SafeSubscriber manages safety and error handling, SynchronizedObserver only does synchronization.

2. New method Exceptions.throwIfFatal(e); checks for special handling.

    public static void throwIfFatal(Throwable t) {
        if (t instanceof OnErrorNotImplementedException) {
            throw (OnErrorNotImplementedException) t;
        }
        // values here derived from https://github.com/Netflix/RxJava/issues/748#issuecomment-32471495
        else if (t instanceof StackOverflowError) {
            throw (StackOverflowError) t;
        } else if (t instanceof VirtualMachineError) {
            throw (VirtualMachineError) t;
        } else if (t instanceof ThreadDeath) {
            throw (ThreadDeath) t;
        } else if (t instanceof LinkageError) {
            throw (LinkageError) t;
        }
    }

3. The throwIfFatal call is invoked before the terminal state check so even if onError was already invoked and throws only to end up again passed to onError the throwIfFatal will catch it on the outside of the isFinished check so it will still throw.

    public void onError(Throwable e) {
        // we handle here instead of another method so we don't add stacks to the frame
        // which can prevent it from being able to handle StackOverflow
        Exceptions.throwIfFatal(e);
        if (isFinished.compareAndSet(false, true)) {
            _onError(e);
        }
    }

These changes appear to have resolved the OnErrorNotImplemented and StackOverflow issues reported in #771 and #748

[benjchristensen]

I removed this because it was relying on SynchronizedObserver behavior related to unsubscribe. This operator should not concern itself with unsubscribe from what I can tell. The SafeSubscriber on a full chain will handle this.

[benjchristensen]

Local build is passing ... merging.

BUILD SUCCESSFUL

Total time: 2 mins 36.851 secs

[cloudbees-pull-request-builder]

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