subscribe() insists on throwing already-handled errors

[mattflix]

RxJS version:
5.5.8

Code to reproduce:

try {
  Rx.Observable
    .create(
      observer => {
        throw `trantrum`
      }
    )
    .catch(
      error => {
        console.log(`totally handled the error: ${ error }`);
        return Rx.Observable.empty();
      }
    )
    .subscribe();
}
catch (error) {
  console.log(`subscribe() blew up anyway: ${ error }`);
}

Expected behavior:

totally handled the error: trantrum

Actual behavior:

totally handled the error: trantrum
subscribe() blew up anyway: trantrum

Additional information:
This behavior seems to have been introduced in 5.5.6 with change 541b49d by @jayphelps. Versions < 5.5.6 exhibit the expected behavior.

If the Observable.create() call is replaced with Observable.defer() (that still synchronously throws an error), we get the expected behavior, even with versions >= 5.5.6.

If this new 5.5.6 behavior is expected, it seems completely surprising, and very painful to deal with. Why create observables with supposedly self-contained error-handling logic, and yet still have to litter code with try/catch blocks?

[jayphelps]

Whoops! 🤡 Thanks for reporting and providing a very good minimal reproduction!

What ever happens, whoever changes the code should be very careful as 541b49d fixed a ton of people who's uncaught errors were otherwise being swallowed silently, which is IMO an even worse bug. Hopefully test coverage will protect us, but just in case!

[mattflix]

See also #3561.

[jayphelps]

@mattflix is it not a duplicate?

[mattflix]

@jayphelps, perhaps so, but #3561 requires an asynchronous element. This one does not. Also, the problematic behaviors were introduced in different versions of RxJS.

Feel free to dupe if you feel the underlying cause is the same. I just don't want both variants to get lost.

(I was recently bitten by BOTH variants in an attempt to upgrade to 5.5.8, from 5.1.1. Interim solution for me was to only upgrade only as far as 5.4.3.)

[cartant]

The two issues refer to the same problem; the only difference is the stack frame from which the error is re-thrown.

The problem is that the error caught in _trySubscribe is re-thrown - because syncErrorThrown is set to true - even though it's reported via sink.error.

The (already very helpful) repro can be made even simpler, as the catch operator can be removed. This snippet will still reproduce the problem:

try {
  Rx.Observable
    .create(observer => { throw `trantrum`; })
    .subscribe({
      error: err => console.log(`subscribe() received the error: ${ err }`)
    });
} catch (err) {
  console.log(`subscribe() blew up: ${ err }`);
}

The snippet's output will be:

subscribe() received the error: trantrum
subscribe() blew up: trantrum

Clearly, if the error is reported via the sink's error method, but is re-thrown anyway, there's no way that the catch operator can prevent the re-throw, as the catch operator is the sink.

[cartant]

Regarding defer, it doesn't exhibit the problem because it has an internal try/catch that sees thrown errors reported to the observer's error method. So _trySubscribe doesn't catch anything and its 'double handling' of a caught error is avoided.

[benlesh]

Just to simplify, as I answered this in the other issue:

WIthin the function passed to the Observable constructor (aka Observable.create), developers are expected to handle their own errors appropriately and report those to observer.error as needed. This is done to provide maximum flexibility to the constructor (i.e. what if you want the subscribe to throw?).

We'd have to have some additional serious design discussions about changing this behavior.

Until then, you can solve your issue by either using try { /*... */ } catch (err) { observer.error(err); } or defer, or even just throwError if you really want an errored observable.

[mattflix]

@benlesh, your comment doesn't seem to explain why the same error is delivered BOTH to the .catch() callback and also to the catch block. (See my repro case.)

Also, I would never want subscribe() to throw. I understand how people can have different opinions about that, but it doesn't seem like very "Rx-ish" to me. (And I've been using Rx for several years in C#, Java, and JavaScript.) This behavior seems surprising, confusing, and inconsistent.

As general comment: The error-handling flow in RxJS has seemingly become mysterious and voodoo-like. Trying to understand and predict what it will do has become frustrating and, in some real examples in my professional work, detrimental to the stability of production code. We now look upon moving to any new version of Rx (major and minor) with great suspicion.

[mattflix]

I would also like to point out that, even if I wanted subscribe() to throw (for the purpose of receiving the error in the catch block for handling) if any asynchronous element is introduced into the observable chain between my subscribe() and the create() callback, then the error will be delivered to the current unhandled error handler instead (if one exists; otherwise the program may simply terminate).

Trying to write generalized Rx code that can reliably and predictably handle errors, where other observables may be injected (beyond local control) into the sequence being subscribed to, basically becomes impossible in some cases.

Putting a try/catch around subscribe() is thus just wishful thinking... it may be meaningful, or it may not. On the flip-side, creating an unhandled error handler (if one's code is even in the position to do so) that can somehow "do the right thing" in all contexts is also clearly a non-starter.

[mattflix]

@jayphelps, you seemed to think the repro highlighted a real problem. Has @benlesh missed something about this issue? Do you agree the current behavior is kosher? (Both handlers being called for a single error.)

[jayphelps]

@mattflix I haven't thought hard enough into all the angles to know whether this behavior is in fact ideal, but if it did get introduced as part of my commit then I would assume it's a bug because I did not intend to introduce it.

Unless I'm doing something wrong, it doesn't appear that v6 of rxjs has this behavior, which lends credence to it being a bug too. https://stackblitz.com/edit/rxjsv6errorexamplethingy?file=index.js

[cartant]

IMO, this is somewhat related to the discussions in #3469.

That is, there will be some situations in which an error cannot be reported via error - if the subscriber is stopped - and others where is can be reported. If it can be reported via a call to the error, I don't see why it should be re-thrown.

The difference between this behaviour and the v6 behaviour is that here, the implementation always re-throws - which can lead to the 'double handling' of errors if the error method is able to report the error.

In v6, the implementation only reports via the error method - which can lead to errors being swallowed.

I'd definitely call this a bug.

[hiepxanh]

what if I want catch from behavior subject ? that make me confuse

[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.