Errors swallowed in stopped subscribers

[cartant]

RxJS version: 6.0.0-beta.1

Code to reproduce: See #3444

Expected behavior:

Errors should not be swallowed.

Actual behavior:

Errors are swallowed.

Additional information:

The Subscriber.prototype.error implementation:

error(err?: any): void {
  if (!this.isStopped) {
    this.isStopped = true;
    this._error(err);
  }
}

swallows any errors that are passed to a stopped subscriber.

See #3444 for a repro. The internal error is swallowed because the subscriber to which it is passed is stopped at the time the error occurs.

[benlesh]

This is by design. If a Subscriber has already been notified of completion or error, we do not want it to be notified again in a reentrant way.

[cartant]

But should the error be completely ignored - i.e. not re-thrown or otherwise ensured to be unhandled? At the moment, the implementation of Subscriber.prototype.error hides the implementation error in #3444 - calling next on a null property.

[cartant]

The problem is in the implementation of _trySubscribe:

protected _trySubscribe(sink: Subscriber<T>): TeardownLogic {
  try {
    return this._subscribe(sink);
  } catch (err) {
    if (config.useDeprecatedSynchronousErrorHandling) {
      sink.syncErrorThrown = true;
      sink.syncErrorValue = err;
    }
    sink.error(err);
  }
}

If there is a subscriber in the list of sinks - the subscribers chained together using the destination property - that happens to be closed/stopped, the error will be swallowed.

The only way that I can see that this could be fixed would be to walk the chain of subscribers and if a closed/stopped subscriber is found, rethrow the error or call hostReportError depending upon the useDeprecatedSynchronousErrorHandling configuration setting.

[benlesh]

So to clarifiy, you're saying that in this case:

new Observable(subscriber => {
  subscriber.error(new Error('first one'));
  subscriber.error(new Error('second one'));
})
.subscribe({
  error(err) { console.log(`handled ${err.message}`); }
});

We should see the first one log as handled, and the second one should be rethrown (in another callstack)?

[cartant]

I've just realised that I've replied to the above comment - which I read in an email - here, in the related PR conversation.

[cartant]

This issue probably should be closed.

I'll create a new issue that better describes the repro and motivation - if I can't recall it myself, this issue is unhelpful - and will include a snippet similar the one in this comment that instead reports errors to the console if they cannot be reported to the observer.

[cartant]

Closing this in favour of #3803 - which is, hopefully, a more clear description of the problem.

[cartant]

@benlesh I've closed this in favour of #3803. This issue still has the "Agenda Item" and "Discussion" labels. My recollection from the last meeting was that consensus was reached regarding the reporting of errors to the console. If that's not the case, you might want to apply the labels to #3803.