fix(Subscriber): creates subscriber throwable by default

[kwonoj]

Description:

I have gut feeling this might not be appropriate fix, feel free to close.

This PR ensure subscriber created internally have nested parents (i.e CombineLatestSubscriber has Mergemap as parents, etcs) are having proper syncErrorThrown behavior. Previously it bypasses setting syncError* property as well as wrapping it into safesubscriber, allows some of cases errors are not thrown but just swallowed.

Related issue (if exists):

• closes Errors sometimes not rethrown #2813

[kwonoj]

/cc @benlesh

[kwonoj]

lol this makes whole test cases fail, not sure why I couldn't caught in local test.

[rxjs-bot]

	Messages
📖

CJS: 1342.5KB, global: 737.3KB (gzipped: 137.9KB), min: 144.6KB (gzipped: 30.7KB)

Generated by 🚫 dangerJS

[kwonoj]

even if this is accepted it'll be probably 5 only and not being cherrypicked into next branch as planned breaking changes.

[benlesh]

I'm not sure about this one... it seems like it will make syncError tracking occur within operators, which I'm pretty sure isn't necessary (which is why the current code avoids it explicitly).

Either way, error rethrowing is going away in 6.0 to reach compliance with the proposal, and to prevent some really, really nasty errors like:

// imagine we're shipping this off to a few third parties
const source$ = Observable.interval(1000).share();

// third party 1
source$.map(x => x + x).subscribe(x => console.log(x));

// third party 2
source$.filter(x => {
  if (x > 3) throw new Error('haha');
  return x <= 3;
})
.subscribe(x => console.log(x));

// third party 3
source$.mergeMap(x => Observable.timer(x))
  .subscribe(x => console.log(x))

... what's going to happen above is on the fifth emission, the number 4 hits "third party 2", which throws an error, the error propagates down to the final subscriber, is unhandled, then is rethrown. The rethrown error unwinds the stack back to the loop in the share() that is notifying everyone for the multicast. That loop breaks, stopping notifications from being sent to anyone else. This is producer interference.

There are other reasons too... Like how are you going to try { } catch { } an error that is thrown asynchronously?

In the end, in v6, we will have a notification channel for unhandled errors, and we'll need to remove the rethrowing behavior.

[kwonoj]

I'm not sure about this one

Yes, reason I mentioned in PR I have gut feeling this might not be appropriate fix, feel free to close. :)

it seems like it will make syncError tracking occur within operators, which I'm pretty sure isn't necessary

Most cases it's true, refer issue #2813 for some higher order observable cases with asynchronously scheduled. As I said already above I have feeling this might not appropriate fix, but we do currently have completely swalloing exception and doesn't deliver it into top level observer.

In the end, in v6, we will have a notification channel for unhandled errors, and we'll need to remove the rethrowing behavior.

Absolutely, issue should be v5 targeted fix as mentioned even if this is accepted it'll be probably 5 only and not being cherrypicked into next branch as planned breaking changes.

[benlesh]

I'm trying to decide if this solves #3183 or not.

[kwonoj]

@benlesh worth peek #3161 as well, I have guess that might be more feasible to try out. This PR is too ad-hoc honestly.

[jayphelps]

did #3161 solve the same problem?

[benlesh]

Yes, #3161 solved the same problem. I think we can close this. @kwonoj, please verify, and open it up if I'm mistaken.

[coveralls]

Coverage remained the same at ?% when pulling 9309bae on kwonoj:fix-syncthrowable into 2e576dc on ReactiveX:master.

[lock]

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.