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

CompositeException fix for Android #1431

Merged
merged 2 commits into from
Jul 21, 2014

Conversation

tomrozb
Copy link
Contributor

@tomrozb tomrozb commented Jul 11, 2014

Fixes #1405

  • revert changes from f4ae92a
  • remove duplicated causes in stack trace chain

@cloudbees-pull-request-builder

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

@benjchristensen
Copy link
Member

This will take a while to review as those changes were done for good reason.

@tomrozb
Copy link
Contributor Author

tomrozb commented Jul 11, 2014

Maybe I should provide some explanation about what was wrong with the previous implementation.

Here's how most of Java programmers will rethrow an exception if not handled:

        Observable.create((Subscriber<? super Object> subscriber) ->
                subscriber.onError(new RuntimeException("ex1")))
                .observeOn(AndroidSchedulers.mainThread())
                .subscribeOn(Schedulers.io()).subscribe(
                object -> {
                    // ...
                }, error -> {
                    // handle some exceptions
                    ...
                    // rethrow unhandled
                    throw new RuntimeException("ex2", error);
                }
        );

Now we have CompositeException with two nested exceptions:

  • ex1
  • ex2 which cause is ex1

The previous implementation will create a loop in this situation:

OnErrorFailedException
   CompositeException
      ex1
         ex2
             ex1
                 ex2
                     ...

I've provided test for this scenario and additional method to remove throwables which are causes of another throwable. It means ex1 will not be attached as a cause of the CompositeException because it is a cause of ex2 so it will be automatically attached with ex2 and printed in the stack trace output.

OnErrorFailedException
   CompositeException
         ex2
             ex1

@benjchristensen
Copy link
Member

/cc @mattrjacobs Matt, can you get involved here (now that you're back) since you have the most recent history and context in this code. Discussion at #1405

@mattrjacobs
Copy link
Contributor

Given the new constraint that Android does something unexpected (to me anyway) with the set of printStackTrace methods, it's probably preferable to modify the structure of CompositeException, so that any arbitrary output of a CompositeException is correct. Relying on the implementation details of precisely how Android does the printStackTrace doesn't protect us from any other edge cases.

So I generally think the sort of change proposed by @tomrozb is good - I'm validating what the output looks like in our prod env now. Thanks @tomrozb for the PR.

@mattrjacobs
Copy link
Contributor

I'm merging this now, thanks for the clear explanation, and fix with a test, @tomrozb.

mattrjacobs added a commit that referenced this pull request Jul 21, 2014
CompositeException fix for Android
@mattrjacobs mattrjacobs merged commit 0661d00 into ReactiveX:master Jul 21, 2014
@benjchristensen
Copy link
Member

Thanks @mattrjacobs and @tomrozb

@tomrozb tomrozb deleted the composite-exception branch July 22, 2014 20:23
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.

4 participants