catchError does not wait for the target observable to complete

[alexand7u]

Bug Report

Current Behavior
catchError returns the data emitted only by atomic observables, who return the value immediately, rather that waiting for the target observable to complete.

Reproduction
This does not represent the flow on which I had the issue is just a simplification of the problem.

import { Observable } from 'rxjs';
import { catchError, delay } from 'rxjs/operators';

const observable$ = new Observable(observer => {
    observer.error("error thrown")
    observer.complete()
})

const observableWithDelay$ = new Observable(observer => {
    observer.next("delayed data")
    observer.complete()
}).pipe(delay(3000))

observable$.pipe(
    catchError(err => observableWithDelay$)
)
.subscribe(
    data => console.log(`the data ${data}`),
    err => console.log(`the error ${err}`),
    () => console.log('end')
)

The result is :

end

rather than

dalayed data
end

Expected behavior
catchError should act as a switchMap in case of error.

Environment

• Runtime: Node, Typescript 3.3.3
• RxJS version: RxJS version: 6.5.2

By the look of the documentation from https://rxjs-dev.firebaseapp.com/api/operators/catchError this looks like a bug because catchError can be used as a retry.

[cartant]

I think the problem is rather subtle. It's because complete is called after error in observable$. If it's not called, it will behave as you are expecting:

const observable$ = new Observable(observer => {
  observer.error("error thrown");
  // observer.complete();
});

So it's not the catchError that effects this behaviour; it's the source.

Essentially, the error is being caught - and dealt with - but the source then completes. I guess this could be deemed a bug, though. Once the source emits an error, that should be the end of things as far as the source's subscribers are concerned. They should not be receiving further notifications from the source.

[bgotink]

I was investigating this, and the retry operator has a related problem:

const syncSource = new Observable(subscriber => {
  subscriber.error('err');
  subscriber.next(1);
  subscriber.complete();
});

const asyncSource = new Observable(subscriber => {
  Promise.resolve().then(() => {
    subscriber.error('err');
    subscriber.next(1);
    subscriber.complete();
  });
});

syncSource.pipe(retry(2)).subscribe(console.log, console.error, () => console.info('complete'));
// logs 'err' to console.error

asyncSource.pipe(retry(2)).subscribe(console.log, console.error, () => console.info('complete'));
// logs '1' to console.log, then 'complete' via console.info

In short, every operator that recycles the subscriber (i.e. doesn't set subscriber.isStopped to true on error or complete) leads to this bug.

[cartant]

I'm no longer sure that this is a bug. If you read the Observable Contract, you will find this:

An Observable may make zero or more [next] notifications, each representing a single emitted item, and it may then follow those emission notifications by either [a complete] or an [error] notification, but not both. Upon issuing [a complete] or [error] notification, it may not thereafter issue any further notifications.

I think that the source in this example is behaving in a manner that is contrary to the contract and the unexpected behaviour should not be surprising.

I would never call complete or next after error and I think that it is incorrect to do so.

[kwonoj]

I agree with @cartant here, error / complete is both terminal condition and observable should have one condition only. Pretty much same as resolve / reject, you can't resolve after reject.

[bgotink]

In promises the following code only resolves, ignoring the call to reject.

console.log(await new Promise((resolve, reject) => {
  resolve('hello world');
  reject('bye cruel world');
}));

In rxjs's observables the same happens: Subscriber has the isStopped property that stops future calls to next, complete or error from going through if a terminal condition is reached.
There's one but: certain operators result in a subscriber that doesn't always show this behaviour.

The existence of isStopped seems to suggest that the intention existed to block this wrong behaviour from causing shenanigans.

[cartant]

@bgotink I'm wary that this might affect anything that's using an InnerSubscriber. In particular, I'm wondering whether weird behaviours might be effected by non-compliant sources that call next after complete if they have flattening-operator subscribers.

Basically, this appears to be a gap in the defensive programming to which you have referred. I'm still not inclined to call this a bug. It's entirely possible that this might not be solvable without some fundamental architectural changes.

ATM, I'm more inclined to write a linting rule to catch (at least some of) these types of non-compliant sources.

[benlesh]

The linting rule is a good idea.... however, a dead subscriber is supposed to stay dead. It's a violation of our contract, and I'm inclined to call this a bug. Interestingly, in this case, this wasn't an issue most of the time back when it was created in v5, because in v5 we synchronously threw the error if it was unhandled, which means that the function block with the superfluous nexts and other calls to subscriber would have erred out.

That said, this is an edge case bug, because people that are hitting this are doing bad things.. It's been 5 years since this could have happened, and 2 since we stopped sync error rethrowing, and this is the first time I've seen this. So we should prioritize accordingly. This is likely something we can resolve when we get to a more comprehensive rewrite in v8.

[cartant]

Regarding linting rules, there is:

• rxjs-no-redundant-notify in rxjs-tslint-rules; and
• no-redundant-notify in eslint-plugin-rxjs.

[xp44mm]

i suggest every event from source observable emit can represent as a fully wraper object:

let yieldEventPayload = {
    kind: "next" | "complete" | "error",
    value: any,
    done: false | true,
    error: null | new Error(),
}

it compatible IteratorResult. the value, done, or error property can pick out of one of them.