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

MLIBZ-2323: Thrown error in onNext() in observable causes silent observable unsubscribe #302

Merged
merged 3 commits into from May 31, 2018

Conversation

Projects
None yet
2 participants
@thomasconner
Contributor

thomasconner commented May 29, 2018

Description

When an error in the onNext() function is thrown, the error is silently handled by the SDK and an unsubscribe from the observable is called, thus leaving the developer without notification that something went wrong.

A running sample describing the above can be found here. Without a try-catch block, trying to access a property from the returned entities will cause an error as the first fetched result would be from the localStorage and thus an empty array. The thrown error will not be shown to the developer causing a silent unsubscribe from the observable.

Changes

  • Don't swallow errors caused in the onNext() function for an Observable

Tests

  • Added test to check that an error is not swallowed when created in the onNext() function

@thomasconner thomasconner self-assigned this May 29, 2018

@thomasconner thomasconner requested a review from vinaygahlawat May 29, 2018

@vinaygahlawat

Overall LGTM, but just have one question to be answered before approval.

@@ -128,7 +128,7 @@ class SafeSubscriber extends Subscriber {
try {
fn.call(this._context, value);
} catch (err) {
this.unsubscribe();
this.error(err);

This comment has been minimized.

@vinaygahlawat

vinaygahlawat May 29, 2018

Contributor

Makes sense to throw an error here. But are there certain cases where we should still unsubscribe, or is this now something the developer should handle completely?

This comment has been minimized.

@thomasconner

thomasconner May 31, 2018

Contributor

It will still unsubscribe after the error callback is called.

@vinaygahlawat

LGTM

@thomasconner thomasconner merged commit c0889a0 into master May 31, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@thomasconner thomasconner deleted the MLIBZ-2323 branch May 31, 2018

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