Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error Handling: OnErrorNotImplemented and java.lang.Error #839

Merged
merged 1 commit into from
Feb 8, 2014

Conversation

benjchristensen
Copy link
Member

@benjchristensen
Copy link
Member Author

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?

- ReactiveX#748 (comment)
- ReactiveX#771
- ReactiveX#789

- SynchronizedObserver is for synchronization, not error handling or contract enforcements, that's the job of SafeSubscriber
- Removed some unit tests that were asserting unsubscribe behavior that relied on SynchronizedObserver. They were testing something they are not responsible for.
@benjchristensen
Copy link
Member Author

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;
        }
    }
  1. 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

@@ -269,33 +269,6 @@ public void testMergeList() {
}

@Test
public void testUnSubscribe() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

Local build is passing ... merging.

BUILD SUCCESSFUL

Total time: 2 mins 36.851 secs

benjchristensen added a commit that referenced this pull request Feb 8, 2014
Error Handling: OnErrorNotImplemented and java.lang.Error
@benjchristensen benjchristensen merged commit d56b1b9 into ReactiveX:master Feb 8, 2014
@cloudbees-pull-request-builder

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

@benjchristensen benjchristensen deleted the error-handling branch February 8, 2014 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants