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

finally() not consistently called in case of unhandled error #3171

Closed
trevorade opened this issue Dec 7, 2017 · 5 comments
Closed

finally() not consistently called in case of unhandled error #3171

trevorade opened this issue Dec 7, 2017 · 5 comments

Comments

@trevorade
Copy link

trevorade commented Dec 7, 2017

RxJS version: 5.5.3

Code to reproduce:

Please reference this JSFiddle for an interactive reproduction of this issue:
http://jsfiddle.net/trevorade/gu8v3ma5/

Specifically, I take issue with this:

const subject = new Rx.Subject();

subject
    .finally(() => console.log('First finally called'))
    .subscribe(() => {});

subject
    .finally(() => console.log('Second finally called'))
    .subscribe(() => {});

subject.error('error');

Expected behavior:

"First finally called" and "Second finally called" are both logged.

Actual behavior:

Only "First finally called" is logged. Which implies that the second finally linked to a subscription of the Observable is not called.

Additional information:

Here's a workaround for the problem:

const subject = new Rx.Subject();

subject
    .finally(() => console.log('First finally called'))
    .subscribe(
        () => {},
        () => {});  // This error handler will result in the second finally being called.

subject
    .finally(() => console.log('Second finally called'))
    .subscribe(() => {});

subject.error('error');

Now that the first subscription has an error callback, the second subscription is processed resulting in the second finally callback being called.

The ideal behavior would be that, even if an exception is not handled by a previous subscription, subsequent subscriptions would still be called (including any error handling or finally callbacks).


Referring to the repro demo again: http://jsfiddle.net/trevorade/gu8v3ma5/, there are several combinations of error handling present or absent that I take issue with. I'll explain the issues per combination:

const handleError1 = true;
const handleError2 = true;

True/true works correctly. This is one of the work-around cases.

const handleError1 = true;
const handleError2 = false;

With, true/false both finally callbacks are called which is called. Unfortunately, an exception is surfaced at the location where subscribe.error is called. This may be desirable behavior but may also be confusing. I feel this is a separate issue though.

const handleError1 = false;
const handleError2 = true;

This is a pretty broken case where the second error handler doesn't get a chance to process the error because the first subscription didn't. Also, only the first finally is called, not both.

const handleError1 = false;
const handleError2 = false;

With no error handling incorporated into the subscriptions, only the first finally is called. This is also unexpected and undesirable.

@martinsik
Copy link
Contributor

A similar thing is discussed here: #3004.

The actual behavior is what I would expect to happen in my opinion.

@trevorade
Copy link
Author

trevorade commented Dec 7, 2017

@martinsik, OK. I personally find this behavior surprising. finally should always be called. That's the nature of finally in most languages that I'm aware of.

@benlesh
Copy link
Member

benlesh commented Jan 24, 2018

@trevorade This is a byproduct of "producer interference"

The issue is that when you don't have an error handler in your subscribe, RxJS 5 and under will rethrow the error synchronously, which unwinds the stack back to the loop inside of the Subject that is notifying all observers, killing it.

In RxJS 6 (currently in master), this is avoided by using a strategy called HostReportErrors, that basically rethrows the error in a setTimeout, giving it it's own callstack.

Again, this is only in the case were an error is completely unhandled.

@benlesh benlesh closed this as completed Jan 24, 2018
@benlesh
Copy link
Member

benlesh commented Jan 24, 2018

Closing, because it's fixed in v6, and we can't change it in v5 without making a breaking change for others.

@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

No branches or pull requests

3 participants