Exception not thrown when maximum call stack is exceeded

[reline]

Bug Report

Current Behavior
When subscribing to a stream that emits errors indefinitely and returning the original source when catching said error, eventually the maximum call stack is exceeded. I expect an exception to be thrown in this case, but it is currently swallowed and printed to the console.

Reproduction

import { of } from 'rxjs';
import { map, catchError } from 'rxjs/operators';
import { TestScheduler } from 'rxjs/testing';

const observable = of(4).pipe(
  map(n => {
      if (n === 4) {
        throw 'four!';
    }
    return n;
  }),
  catchError((err, source) => source),
);
 
const testScheduler = new TestScheduler((actual, expected) => {
  // using jest
  expect(actual).toMatchObject(expected);
});
 
// This test should fail due to Maximum call stack size exceeded
it('should fail', () => {
  testScheduler.run(helpers => {
    const { expectObservable } = helpers;
    expectObservable(observable).toBe('-');
  });
});

 PASS  __tests__/callstack-test.ts
  ✓ should fail (14ms)

  console.warn node_modules/rxjs/internal/Observable.js:55
    RangeError: Maximum call stack size exceeded
        at new SourceMapConsumer (/dev/node_modules/source-map-support/node_modules/source-map/lib/source-map-consumer.js:17:22)
        at mapSourcePosition (/dev/node_modules/source-map-support/source-map-support.js:190:14)
        at wrapCallSite (/dev/node_modules/source-map-support/source-map-support.js:367:20)
        at Function.prepareStackTrace (/dev/node_modules/source-map-support/source-map-support.js:416:39)
        at prepareStackTrace (internal/errors.js:37:29)
        at formatError (internal/util/inspect.js:907:19)
        at formatRaw (internal/util/inspect.js:703:14)
        at formatValue (internal/util/inspect.js:591:10)
        at inspect (internal/util/inspect.js:221:10)
        at formatWithOptions (internal/util/inspect.js:1693:40)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        1.302s, estimated 2s

Expected behavior
I would like the exception to be thrown and for this test to fail.

Environment

• Runtime: Node v12.16.1
• RxJS version: 6.5.4

Additional context/Screenshots
I see in #3803 that the warn was added instead because throwing new errors would have been a breaking change. Could this be considered for 7.x?

[cartant]

This is by design - to avoid a breaking change. See #4089

@benlesh in which major version will it be okay to change this to throw using hostReportError?

Also, even when hostReportError is used, your test will probably not behave as you expect because the error will be thrown asynchronously.

[benlesh]

Observables should always be assumed asynchronous in tests, because we have to handle our errors as such. Anytime someone does a try { observable.subscribe(); } catch (err) {} they're relying on luck, as the observable should be treated as a black box.

The ONLY source of truth for observable errors should be the error handler in subscriptions.

The fix for the max call stack error here might be to catch at the subscription point. These sorts of things are weird to defend against.

Also of note: infinitely retrying or repeating a synchronous observable is probably a bad idea.

[benlesh]

Yeah... @cartant, it looks like we're not catching errors that happen during subscription when subscribing to observables that were created via lift. Other observables are using _trySubscribe, which will catch errors and send them down the proper path.

[benlesh]

Looking into this, I think this is going to add a bit of weight to an already bloated hot-path.

As it stands, we have a lot of logic around error handling during subscription that is just there for legacy reasons to keep us from breaking our partners at Google. (see _enable_super_gross_mode_that_will_cause_bad_things)... That truly MUST go by v8, and I think that will be the appropriate time to fix this, since it's truly an edge case, IMO.

[benlesh]

I think with this one, what we want to do is probably add some tests that we skip for now, but have resolved by version 8, when we do a more comprehensive refactor of the library.