Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(subscribe): don't swallow internal errors #4089

Merged
merged 22 commits into from
Sep 12, 2018

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Sep 4, 2018

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
Copy link
Contributor

pertrai1 commented Sep 4, 2018

kaboom - 👍 LGTM

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return true;
}

function consoleReportError(err: any): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cartant
Copy link
Collaborator Author

cartant commented Sep 4, 2018

@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
Copy link

ci-reporter bot commented Sep 5, 2018

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
Copy link

coveralls commented Sep 5, 2018

Coverage Status

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use isTrustedSubscriber

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should always be a Subscriber of some sort.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

benlesh commented Sep 7, 2018

Looking good, just a couple of nits.

@cartant
Copy link
Collaborator Author

cartant commented Sep 7, 2018

@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
Copy link
Collaborator Author

cartant commented Sep 8, 2018

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

@benlesh benlesh merged commit 9b4b2bc into ReactiveX:master Sep 12, 2018
@aniruddhadas9
Copy link

which version of RxJs will this be included?

@lock lock bot locked as resolved and limited conversation to collaborators Oct 19, 2018
@cartant cartant deleted the issue-3803 branch September 24, 2020 07:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants