Skip to content

1.x: Add missing error handler call in Completable #3938

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

Closed
wants to merge 1 commit into from

Conversation

bryant1410
Copy link

No description provided.

@akarnokd
Copy link
Member

Why do you think it's missing? In operators, only undeliverable exceptions are supposed to be sent to the global handler.

@bryant1410
Copy link
Author

I'm not pretty sure if this call to the handler goes there. But it arises from using the Retrofit's RxJava adapter, and realizing that Completable's endpoints errors are not caught by an error handler, while Observable's and Single's are.

@akarnokd
Copy link
Member

Interesting, could be that Completable doesn't use SafeSubscriber, which is responsible to deliver some errors thrown by onXXX methods. To be sure, could you add a unit test that demonstrates how Single gets your exception and how Completable isn't?

@bryant1410
Copy link
Author

Okey

@bryant1410
Copy link
Author

bryant1410 commented May 14, 2016

I think this comment is related: Completable.java#L1968. From what I see, Completable, in a different way to Observable and Single, does calls to the error handler plugin by itself, instead of delegating it to a SafeCompletableSubscriber (which btw doesn't exist). Why there is no safe subscriber for Completable and why is this done this way?

The call to the handler is missing from onError in subscribe(Action1<? super Throwable>, Action0), subscribe(CompletableSubscriber) and subscribe(Subscriber) methods, but subscribe() and subscribe(Action0) have it. This should be refactored to have just one place for calling the handler, as in Observable and Single.

@bryant1410 bryant1410 changed the title Add missing error handler call in Completable 1.X: Add missing error handler call in Completable May 14, 2016
@bryant1410 bryant1410 changed the title 1.X: Add missing error handler call in Completable 1.x: Add missing error handler call in Completable May 14, 2016
@akarnokd
Copy link
Member

Completable was designed with a modern mindset where end-CompletableSubscribers are not expected to throw from the onXXX methods, but apparently, there was no safeSubscribe() added to the API. The current 1.x convention is to have subscribe() do additional safeguards and have an unsafeSubscribe() as direct as possible.

@akarnokd
Copy link
Member

See #3942 for other forms of handling errors.

@bryant1410 bryant1410 closed this May 14, 2016
@zsxwing
Copy link
Member

zsxwing commented Jun 1, 2016

Closing via #3942

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants