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

subscribe onError swallows any exception thrown #2618

Closed
DerekBriske opened this issue May 26, 2017 · 9 comments · Fixed by #2796
Closed

subscribe onError swallows any exception thrown #2618

DerekBriske opened this issue May 26, 2017 · 9 comments · Fixed by #2796

Comments

@DerekBriske
Copy link

DerekBriske commented May 26, 2017

I have an Angular 4 application that use rxs.

The error handler when added to the subscribe is swallowing an exception if one is thrown from within the handler. In version 5.0.1, exceptions thrown from the error handler bubbled out to Angular's global error handler.

It appears this was broken with version 5.1, If I change the version in my plunker to 5.1, it behaves as it does in the latest version.

.subscribe(
  result => {
    //Throwing an error here gets picked up by the global error handler
    throw new Error('break in on next');
    this.result = result.artists.items; },
  err => { 
    console.log('subscribe on error in on next');
    //This error does bubble out to the global angular error handler,
    // since it was thrown from the onNext
    throw new Error('Error in on next here'); 
  },
  () => { console.log('Completed'); }
);

I posted the following on stack overflow.
https://stackoverflow.com/questions/44187493/rxjs-angular-4-throw-error-in-subscribe-onerror-not-bubbling-out-to-custom-glo

RxJS version: 5.0.1

Code to reproduce:

where it works: 5.0.1 - https://plnkr.co/edit/ZMhTNbBfWaz4Y0TiI0jR?p=preview

where it is broken: latest version of rxjs - https://plnkr.co/edit/BI4njaMbtR7pfplQvgru?p=preview

Expected behavior:
Exception thrown from the onError of a subscription should be thrown out. In my example it would be caught by Angular's global error handler.

Actual behavior:
Exception is swallowed by the subscription and not thrown out.

@kwonoj
Copy link
Member

kwonoj commented May 26, 2017

I'm not sure if this was intended changes or side effects, guess related to sinks in our subscriber. @trxcllnt - thoughts?

@kwonoj
Copy link
Member

kwonoj commented May 29, 2017

Looking into it, I think this is dupe of #2565 has changes at #2626.

@jayphelps
Copy link
Member

jayphelps commented Jun 21, 2017

I am still seeing problems as well, even with 5.4.1 and 6.0.0-alpha

https://jsbin.com/dukixu/edit?html,js,output

const input$ = new Rx.Subject();

input$
  .mergeMap(() => Rx.Observable.of(1, 2))
  .subscribe(d => {
    throw new Error('some error');
  });

input$.next();

Error correctly thrown in 5.0.3 , started being swallowed in 5.1.0 and continuing to latest versions.

Note that if you change Rx.Observable.of(1, 2) to just one Rx.Observable.of(1) it propagates the error correctly. Same if you use an Observable.of instead of a Subject as the source. This appears to be because _isScalar observables don't get subscribed to in subscribeToResult so it doesn't hit the buggy path.

Cc/ @benlesh

@haf
Copy link

haf commented Jun 25, 2017

Ping @kwonoj – this is causing our app to just plain halt.

@jayphelps
Copy link
Member

@haf you can back down to 5.0.3 to find the bug in your code that is throwing the error then upgrade back up after you've fixed it. This should not be a blocker

@jayphelps
Copy link
Member

jayphelps commented Jun 25, 2017

Also, support is provided entirely voluntary. Pinging us repeatedly just burns us out and makes us not want to help you. Have you tried finding the bug in Rx? Being able to find bugs in your dependencies is a very powerful ability for self sustainability.

Edit: this is my personal opinion, not speaking for kwonoj

@haf
Copy link

haf commented Jun 25, 2017

@jayphelps I didn't realise you were a maintainer of this project, so I wasn't pinging you, but kwonoj who hasn't replied. I also haven't repeatedly pinged you, this was the first time.

@jayphelps
Copy link
Member

jayphelps commented Jun 25, 2017

Whoops! I'm not a maintainer anymore. sorry for making it sound like I was. I just contribute. I realize you haven't pinged me or kwonoj directly multiple times, I meant that I had already replied here and included more details of my findings so far from trying to find the issue itself.

My exact wording was not accurate, but you prolly get the idea. 😃

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants