fix(subscribe): don't swallow internal errors by cartant · Pull Request #4089 · ReactiveX/rxjs

cartant · 2018-09-04T12:08:44Z

Description:

This PR reports errors that would otherwise be swallowed - because of a closed subscriber - to the console. (This behaviour is something that bit me again, today, when I was investigating a snippet that effected a stack overflow error in v5, but didn't effect the error in v6. The error was still occurring, of course, it just wasn't being thrown.)

As discussed in the linked issue, calling hostReportError would be preferred - rather than writing to the console - but doing so would be a breaking change.

Related issue (if exists): #3803

pertrai1 · 2018-09-04T14:02:13Z

kaboom - 👍 LGTM

benlesh · 2018-09-04T15:21:55Z

@@ -0,0 +1,34 @@
+import { Subscriber } from '../Subscriber';


Since this is only used in one spot, you can just inline everything in that one spot. It'll perform better, and will be more readable (if a little harder to test)

For tests, I'd rather just have more integration/functional tests of this.

benlesh · 2018-09-04T15:25:01Z

+  return true;
+}
+
+function consoleReportError(err: any): void {


consoleWarn, this is too confusing considering we have a hostReportError function in the library, which is different.

cartant · 2018-09-04T19:44:56Z

@benlesh I've made a bunch of changes inline with your requests:

Removed reportError and used canReportError for readability.
Renamed to consoleWarn.
The canReportError tests now just test the logic - not the reporting.
I've fixed a logic error in the canReportError implementation.
canReportError is no longer recursive.
canReportError error is also used in bindCallback and bindNodeCallback - see bindCallback swallows error if it was thrown after the callback was executed, and the entire function is sycnhronous #3487. (Your mentioning that it's only used in one place reminded me of errors that were swallowed elsewhere.)
There are integration tests for Observable.subscribe, bindCallback and bindNodeCallback that stub console.warn.

ci-reporter · 2018-09-05T00:37:14Z

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of 30440d2. Here's the output:

`npm test`

> @reactivex/rxjs@6.3.2 test /home/travis/build/ReactiveX/rxjs
> cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts "spec/**/*-spec.ts"


/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
spec/util/canReportError-spec.ts(19,27): error TS2345: Argument of type 'Observer<any>' is not assignable to parameter of type 'Subscriber<any> | Subject<any>'.
  Type 'Observer<any>' is not assignable to type 'Subject<any>'.
    Property 'observers' is missing in type 'Observer<any>'.

    at createTSError (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250:12)
    at getOutput (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:358:40)
    at Object.compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:545:11)
    at Module.m._compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:430:43)
    at Module._extensions..js (module.js:663:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:433:12)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at /home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:536:10)
    at Object.<anonymous> (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/bin/_mocha:582:18)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:612:3

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!

Failed build for a78c230

`npm test`

> @reactivex/rxjs@6.3.2 test /home/travis/build/ReactiveX/rxjs
> cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts "spec/**/*-spec.ts"


․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․,,,․․․,․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․���․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․!․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  2584 passing (4s)
  4 pending
  1 failing

  1) Immediate
       should schedule on the next microtask:
     Uncaught Error: kaboom
      at spec/util/canReportError-spec.ts:14:22
      at Context.modified (spec/helpers/testScheduler-ui.ts:206:13)

This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

coveralls · 2018-09-05T01:23:47Z

Coverage increased (+0.2%) to 96.971% when pulling 14a6c61 on cartant:issue-3803 into e3911bc on ReactiveX:master.

Closes ReactiveX#3803

benlesh · 2018-09-07T17:58:43Z

+    const { closed, destination, isStopped } = observer as any;
+    if (closed || isStopped) {
+      return false;
+    } else if (destination instanceof Subscriber || (destination && destination[rxSubscriberSymbol])) {


use isTrustedSubscriber

benlesh · 2018-09-07T17:59:51Z

+ * need to be reported via a different mechanism.
+ * @param observer the observer
+ */
+export function canReportError(observer: ErrorObserver<any>): boolean {


This should always be a Subscriber of some sort.

benlesh · 2018-09-07T18:01:44Z

@@ -0,0 +1,7 @@
+export function consoleWarn(...args: any[]): void {
+  if (console.warn) {


We shouldn't have to check this every time the function is called.

export const consoleWarn = console.warn ? console.warn.bind(console) : console.log.bind(console);

benlesh · 2018-09-07T18:02:45Z

Looking good, just a couple of nits.

cartant · 2018-09-07T21:13:02Z

@benlesh I've made changes inline with your requests:

isTrustedSubscriber has been used.
ErrorObserver<any> has been changed to Subscriber<any> | Subject<any>.

However, I've not changed consoleWarn to test console.warn only once for these reasons:

Testing it and then using bind means that stubbing console.warn won't stub consoleWarn.
I don't see how I could stub consoleWarn itself - as a simple, exported function.
The actual logging to the console will be so much slower that it will make next to no difference, IMO. And it's only going to be in an error condition.

I could replace it with:

export const consoleWarn = console.warn ?
  (...args: any[]) => console.warn(...args) :
  (...args: any[]) => console.log(...args);

But I cannot see that making a discernible difference.

cartant · 2018-09-08T00:03:58Z

console.warn is now called directly. It's called directly elsewhere in the codebase, anyway.

aniruddhadas9 · 2018-09-19T15:46:40Z

which version of RxJs will this be included?

benlesh requested changes Sep 4, 2018

View reviewed changes

cartant force-pushed the issue-3803 branch from 7def30b to aaa804a Compare September 5, 2018 01:08

cartant added 18 commits September 7, 2018 16:57

chore(reportError): implement reportError

9a309c0

fix(subscribe): don't swallow internal errors

ff3f06e

Closes ReactiveX#3803

chore(reportError): remove console.log from test

6801bbd

chore(reportError): test destination is Subscriber

506eb42

test(reportError): use closed observer

ec98f5b

chore(reportError): test subscriber symbol too

936b26b

chore(reportError): fix logic

1ed1339

chore(reportError): rename to consoleWarn

e4959af

test(reportError): stub the console

669ee9c

chore(reportError): fix whitespace

3ee03c0

refactor(reportError): use canReportError instead

9d8cb76

refactor(canReport): remove recursion

bc8f54c

test(subscribe): test internal error reporting

1a9d49a

test(bindCallback): test error reporting

bbfaed0

test(bindNodeCallback): test error reporting

1fd1807

chore(canReport): fix JSDoc

6f40412

chore(test): fix test description

daa02c4

test(canReport): use noop error handlers

ed2a617

cartant force-pushed the issue-3803 branch from aaa804a to ed2a617 Compare September 7, 2018 07:05

benlesh requested changes Sep 7, 2018

View reviewed changes

cartant added 2 commits September 8, 2018 06:58

chore(canReport): use isTrustedSubscriber

1b53211

chore(canReport): restrict observer type

30440d2

cartant added 2 commits September 8, 2018 07:41

chore(canReport): fix tests

d640e9d

chore(canReport): use console.warn directly

14a6c61

benlesh approved these changes Sep 12, 2018

View reviewed changes

benlesh merged commit 9b4b2bc into ReactiveX:master Sep 12, 2018

cartant mentioned this pull request Sep 27, 2018

bindCallback swallows error if it was thrown after the callback was executed, and the entire function is sycnhronous #3487

Closed

lock Bot locked as resolved and limited conversation to collaborators Oct 19, 2018

cartant deleted the issue-3803 branch September 24, 2020 07:08

		@@ -0,0 +1,7 @@
		export function consoleWarn(...args: any[]): void {
		if (console.warn) {

Uh oh!

Conversation

cartant commented Sep 4, 2018

Uh oh!

pertrai1 commented Sep 4, 2018

Uh oh!

benlesh Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

benlesh Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

benlesh Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

cartant commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ci-reporter Bot commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The build is failing

npm test

npm test

This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

Uh oh!

coveralls commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benlesh Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

benlesh Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

benlesh Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

benlesh commented Sep 7, 2018

Uh oh!

cartant commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartant commented Sep 8, 2018

Uh oh!

aniruddhadas9 commented Sep 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cartant commented Sep 4, 2018 •

edited

Loading

ci-reporter Bot commented Sep 5, 2018 •

edited

Loading

`npm test`

`npm test`

coveralls commented Sep 5, 2018 •

edited

Loading

cartant commented Sep 7, 2018 •

edited

Loading