Consider using SuppressedError when invoking a callback in error steps

[domfarolino]

The catch() and inspect() operators have internal observers whose error steps invoke a developer-supplied callback with the given error value they receive. If that callback throws an error, then we must invoke Subscriber's error() method with the error thrown from the callback instead of the original error they intended to surface. This clobbering of the original error is exactly what SuppressedError intends to alleviate (first mentioned in #174), so we should consider using it in those operators.

[Jamesernator]

So this probably shouldn't be done for .catch as it's reasonable that a catch may already be replacing/wrapping the given error with a new error.

I'm now mixed whether it's even worth doing for .inspect given that there is opportunity to react to the given error, though it does seem more reasonable there as an error thrown from .inspect is probably a mistake in most cases.

There is actually one place in .catch though where the error is swallowed, and that's if the return value of the callback fails to be converted to observable:

obs.catch((error) => {
    // We can't catch the error from Observable.from
    return null;
});

so it might be worth doing a SuppressedError here.

[benlesh]

There would not be any errors suppressed in any of the above scenarios. catch behaves much the same as the similarly named method on promise. If there is a problem and the callback throws, that error is sent down the observation chain to be handled. If it's unhandled, then it's reported, similar to any unhandled error elsewhere in the ecosystem.

The only place that someone could have a "suppressed" error would be a situation where somebody called subscriber.error twice. None of the actual methods or operators should have this problem.

The only one that's a little bit weird is finally, but even there if there's an error in the callback, it should be sent to the error handler of the next observer down the chain.

[domfarolino]

There would not be any errors suppressed in any of the above scenarios. catch behaves much the same as the similarly named method on promise. If there is a problem and the callback throws, that error is sent down the observation chain to be handled. If it's unhandled, then it's reported, similar to any unhandled error elsewhere in the ecosystem.

But the idea is that the error catch() was responding to in the first place is essentially eclipsed by catch's error, and SuppressedError gives a way to recover the original error even when the catch callback throws.

[domenic]

I spent some time looking into this more.

The fundamental question is: when should SuppressedError be used across the web platform?

• We know it is used by explicit resource management:

  try {
    using _ = getThingWithThrowingDisposer();
    throw b;
  } catch (e) {
    // e is a SuppressedError containing b as "error" and the error thrown by the disposer as "suppressed".
  }

• We know it is not used by general situations where "an error occurred while handling an error". For example:

  • try {
      try {
        throw a;
      } catch {
        throw b;
      }
    } catch (e) {
      console.assert(e === b); // not a SupressedError wrapping a + b, just b
    }

  • Promise.reject(a)
      .catch(() => Promise.reject(b))
      .catch(e => {
        console.assert(e === b); // not a SupressedError wrapping a + b, just b
      })

So is the observable case more similar to explicit resource management, or to try/catch?

From what I can tell, the reason for introducing SuppressedError in explicit resource management is that the "rewriting" of explicit resource management can introduce errors during cleanup (originating from the Symbol.dispose]() call) which are incidental to the "important" error that is being thrown. And, if we just replaced the originally-thrown error with the disposer error, then developer code would never see the originally-thrown (arguably more-important) error.

Whereas, in the normal try/catch case, developer code was able to see both errors. There's no clear hierarchy to them, besides the temporal order. Often, the developer is intentionally meaning to overwrite a with b (e.g., translating between error types).

I don't quite understand the relevant Observable operators enough to be sure which camp they fall into, but the comments in this thread lead me to believe they're more like the try/catch case. (Except maybe the case @Jamesernator mentions.)

[benlesh]

There's only really one scenario I can think of where an error is (sort of) "suppressed" in an Observable:

const errorsImmediately = new Observable((subscriber) => {
  subscriber.error(new Error('this is emitted'))
  subscriber.error(new Error('this is reported, not emitted'))
})

errorsImmediately.subscribe({
  error: (e) => {
    e.message === 'this is emitted' // true
  }
})

window.addEventListener('error', (e) => {
  e.message === 'this is reported, not emitted'; // true
})

Since the error event is terminal for the subscription, it only fires once. It's possible that it could be a suppressed error, but the problem is then a situation where the second call to subscriber.error happens asynchronously for some reason.

EDIT: This is the same thing too...

new Observable((subscriber) => {
  subscriber.error(new Error('this is emitted'))
  throw new Error('this is reported');
})