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

Exception in onCompleted() not propagate to onError() #630

Closed
ronanM opened this issue Dec 17, 2013 · 3 comments
Closed

Exception in onCompleted() not propagate to onError() #630

ronanM opened this issue Dec 17, 2013 · 3 comments

Comments

@ronanM
Copy link

ronanM commented Dec 17, 2013

When an exception occurred in onCompleted() the exception is not propagate to onError().

Code of rx.operators.SafeObserver

    @Override
    public void onCompleted() {
        if (isFinished.compareAndSet(false, true)) {
            try {
                actual.onCompleted();
            } catch (Throwable e) {
                // handle errors if the onCompleted implementation fails, not just if the Observable fails
                onError(e);
            }
            // auto-unsubscribe
            subscription.unsubscribe();
        }
    }

In our case isFinished is already TRUE, so onError does nothing.

 @Override
    public void onError(Throwable e) {
        if (isFinished.compareAndSet(false, true)) {
           ...
        }
    }
@zsxwing
Copy link
Member

zsxwing commented Dec 17, 2013

That's intentional. According to Rx Contract, A observer should not receive both onCompleted and onError. You need to handle the exception in onCompleted by yourself.

2013-12-17 7 38 00

@ronanM
Copy link
Author

ronanM commented Dec 17, 2013

Ok the contract is clear.

So, I don't understand why in SafeObserver there is this code

catch (Throwable e) {
    // handle errors if the onCompleted implementation fails, not just if the Observable fails
    onError(e);
}

It's in contradiction with the grammar and because isFinished is set to true so this call is useless.

Why not call a global errorHandler or something like that in the catch ?

I don't understand.

Regards,
Ronan.

@benjchristensen
Copy link
Member

Ha, yep that code is pretty useless. Kind of funny ... I'll submit a fix shortly.

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

No branches or pull requests

3 participants