From 9a309c03a9243dbe8f650b7bb05ab3af8b3ab520 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 21:58:39 +1000 Subject: [PATCH 01/22] chore(reportError): implement reportError --- spec/util/reportError-spec.ts | 50 ++++++++++++++++++++++++++++++++ src/internal/util/reportError.ts | 33 +++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 spec/util/reportError-spec.ts create mode 100644 src/internal/util/reportError.ts diff --git a/spec/util/reportError-spec.ts b/spec/util/reportError-spec.ts new file mode 100644 index 0000000000..12d5924400 --- /dev/null +++ b/spec/util/reportError-spec.ts @@ -0,0 +1,50 @@ +import { expect } from 'chai'; +import { noop, Subject, Subscriber } from 'rxjs'; +import { reportError } from 'rxjs/internal/util/reportError'; +import * as sinon from 'sinon'; + +describe('reportError', () => { + it('should report errors to an observer if possible', () => { + const error = new Error('kaboom'); + const subscriber = new Subscriber(noop, e => console.log(e)); + const errorStub = sinon.stub(subscriber, 'error'); + const reportStub = sinon.stub(); + reportError(error, subscriber, reportStub); + expect(errorStub).to.have.property('called', true); + expect(reportStub).to.have.property('called', false); + }); + + it('should not report errors to a stopped observer', () => { + const error = new Error('kaboom'); + const subscriber = new Subscriber(noop); + subscriber.error(error); + const errorStub = sinon.stub(subscriber, 'error'); + const reportStub = sinon.stub(); + reportError(error, subscriber, reportStub); + expect(errorStub).to.have.property('called', false); + expect(reportStub).to.have.property('called', true); + }); + + it('should not report errors to a closed observer', () => { + const error = new Error('kaboom'); + const subject = new Subject<{}>(); + subject.unsubscribe(); + const errorStub = sinon.stub(subject, 'error'); + const reportStub = sinon.stub(); + reportError(error, subject, reportStub); + expect(errorStub).to.have.property('called', false); + expect(reportStub).to.have.property('called', true); + }); + + it('should not report errors an observer with a stopped destination', () => { + const error = new Error('kaboom'); + const destination = new Subscriber(noop); + const subscriber = new Subscriber(destination); + destination.error(error); + const errorStub = sinon.stub(subscriber, 'error'); + const reportStub = sinon.stub(); + reportError(error, subscriber, reportStub); + expect(errorStub).to.have.property('called', false); + expect(reportStub).to.have.property('called', true); + }); +}); diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts new file mode 100644 index 0000000000..5e9b18e553 --- /dev/null +++ b/src/internal/util/reportError.ts @@ -0,0 +1,33 @@ +import { empty } from '../Observer'; +import { ErrorObserver } from '../types'; + +/** + * Reports the error to the ErrorObserver unless the observer is closed or + * stopped - in which case, an alternative reporting mechanism is used. + * @param err the error to report + */ +export function reportError(err: any, observer: ErrorObserver, report?: (err: any) => void): void { + if (canReportError(observer)) { + observer.error(err); + } else { + (report || consoleReportError)(err); + } +} + +function canReportError(observer: ErrorObserver): boolean { + const { closed, destination, isStopped } = observer as any; + if (closed || isStopped) { + return false; + } else if (destination && destination !== empty) { + return canReportError(destination); + } + return true; +} + +function consoleReportError(err: any): void { + if (console.warn) { + console.warn(err); + } else { + console.log(err); + } +} From ff3f06e413ab177b0a6d0048b52130da0468b915 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 22:00:49 +1000 Subject: [PATCH 02/22] fix(subscribe): don't swallow internal errors Closes #3803 --- src/internal/Observable.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/internal/Observable.ts b/src/internal/Observable.ts index d9e8f0c0d5..06ca0dd38a 100644 --- a/src/internal/Observable.ts +++ b/src/internal/Observable.ts @@ -2,6 +2,7 @@ import { Operator } from './Operator'; import { Subscriber } from './Subscriber'; import { Subscription } from './Subscription'; import { TeardownLogic, OperatorFunction, PartialObserver, Subscribable } from './types'; +import { reportError } from './util/reportError'; import { toSubscriber } from './util/toSubscriber'; import { iif } from './observable/iif'; import { throwError } from './observable/throwError'; @@ -226,7 +227,7 @@ export class Observable implements Subscribable { sink.syncErrorThrown = true; sink.syncErrorValue = err; } - sink.error(err); + reportError(err, sink); } } From 6801bbd72db8a9e942615f809521ad5feab9c666 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 22:26:32 +1000 Subject: [PATCH 03/22] chore(reportError): remove console.log from test --- spec/util/reportError-spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/util/reportError-spec.ts b/spec/util/reportError-spec.ts index 12d5924400..6f868db2ea 100644 --- a/spec/util/reportError-spec.ts +++ b/spec/util/reportError-spec.ts @@ -6,7 +6,7 @@ import * as sinon from 'sinon'; describe('reportError', () => { it('should report errors to an observer if possible', () => { const error = new Error('kaboom'); - const subscriber = new Subscriber(noop, e => console.log(e)); + const subscriber = new Subscriber(noop); const errorStub = sinon.stub(subscriber, 'error'); const reportStub = sinon.stub(); reportError(error, subscriber, reportStub); From 506eb42215f10bd7133ebcd20aa08aac90186b41 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 22:41:24 +1000 Subject: [PATCH 04/22] chore(reportError): test destination is Subscriber --- src/internal/util/reportError.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index 5e9b18e553..4385423adb 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -1,4 +1,4 @@ -import { empty } from '../Observer'; +import { Subscriber } from '../Subscriber'; import { ErrorObserver } from '../types'; /** @@ -18,7 +18,7 @@ function canReportError(observer: ErrorObserver): boolean { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { return false; - } else if (destination && destination !== empty) { + } else if (destination instanceof Subscriber) { return canReportError(destination); } return true; From ec98f5b0d92970228fafe54273645c3904cfed3f Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 22:46:37 +1000 Subject: [PATCH 05/22] test(reportError): use closed observer --- spec/util/reportError-spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/util/reportError-spec.ts b/spec/util/reportError-spec.ts index 6f868db2ea..0b188c4062 100644 --- a/spec/util/reportError-spec.ts +++ b/spec/util/reportError-spec.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; -import { noop, Subject, Subscriber } from 'rxjs'; +import { noop, Subscriber } from 'rxjs'; +import { empty } from 'rxjs/internal/Observer'; import { reportError } from 'rxjs/internal/util/reportError'; import * as sinon from 'sinon'; @@ -27,11 +28,10 @@ describe('reportError', () => { it('should not report errors to a closed observer', () => { const error = new Error('kaboom'); - const subject = new Subject<{}>(); - subject.unsubscribe(); - const errorStub = sinon.stub(subject, 'error'); + const closed = { ...empty }; + const errorStub = sinon.stub(closed, 'error'); const reportStub = sinon.stub(); - reportError(error, subject, reportStub); + reportError(error, closed, reportStub); expect(errorStub).to.have.property('called', false); expect(reportStub).to.have.property('called', true); }); From 936b26b5dc47bd7e231a26126e7ca4894ad1a604 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 4 Sep 2018 22:50:57 +1000 Subject: [PATCH 06/22] chore(reportError): test subscriber symbol too --- src/internal/util/reportError.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index 4385423adb..ee5562bd44 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -1,4 +1,5 @@ import { Subscriber } from '../Subscriber'; +import { rxSubscriber as rxSubscriberSymbol } from '../symbol/rxSubscriber'; import { ErrorObserver } from '../types'; /** @@ -18,7 +19,7 @@ function canReportError(observer: ErrorObserver): boolean { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { return false; - } else if (destination instanceof Subscriber) { + } else if (destination instanceof Subscriber || destination[rxSubscriberSymbol]) { return canReportError(destination); } return true; From 1ed1339f569a05c6fb9f41330ea2b079f489a9bd Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 04:41:23 +1000 Subject: [PATCH 07/22] chore(reportError): fix logic --- src/internal/util/reportError.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index ee5562bd44..367a3d5fc7 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -19,7 +19,7 @@ function canReportError(observer: ErrorObserver): boolean { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { return false; - } else if (destination instanceof Subscriber || destination[rxSubscriberSymbol]) { + } else if (destination instanceof Subscriber || (destination && destination[rxSubscriberSymbol])) { return canReportError(destination); } return true; From e4959af6e2f9762f243cedda2929fb1bbc9aa421 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 04:42:01 +1000 Subject: [PATCH 08/22] chore(reportError): rename to consoleWarn --- src/internal/util/reportError.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index 367a3d5fc7..aaeb2740ca 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -11,7 +11,7 @@ export function reportError(err: any, observer: ErrorObserver, report?: (er if (canReportError(observer)) { observer.error(err); } else { - (report || consoleReportError)(err); + (report || consoleWarn)(err); } } @@ -25,7 +25,7 @@ function canReportError(observer: ErrorObserver): boolean { return true; } -function consoleReportError(err: any): void { +function consoleWarn(err: any): void { if (console.warn) { console.warn(err); } else { From 669ee9cc1ba42f6b756d2895da98880f02b01eaa Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 04:46:21 +1000 Subject: [PATCH 09/22] test(reportError): stub the console --- spec/util/reportError-spec.ts | 78 +++++++++++++++++++------------- src/internal/util/reportError.ts | 4 +- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/spec/util/reportError-spec.ts b/spec/util/reportError-spec.ts index 0b188c4062..43934cdcdd 100644 --- a/spec/util/reportError-spec.ts +++ b/spec/util/reportError-spec.ts @@ -6,45 +6,61 @@ import * as sinon from 'sinon'; describe('reportError', () => { it('should report errors to an observer if possible', () => { - const error = new Error('kaboom'); - const subscriber = new Subscriber(noop); - const errorStub = sinon.stub(subscriber, 'error'); - const reportStub = sinon.stub(); - reportError(error, subscriber, reportStub); - expect(errorStub).to.have.property('called', true); - expect(reportStub).to.have.property('called', false); + const consoleStub = sinon.stub(console, 'warn'); + try { + const error = new Error('kaboom'); + const subscriber = new Subscriber(noop); + const errorStub = sinon.stub(subscriber, 'error'); + reportError(error, subscriber); + expect(errorStub).to.have.property('called', true); + expect(consoleStub).to.have.property('called', false); + } finally { + consoleStub.restore(); + } }); it('should not report errors to a stopped observer', () => { - const error = new Error('kaboom'); - const subscriber = new Subscriber(noop); - subscriber.error(error); - const errorStub = sinon.stub(subscriber, 'error'); - const reportStub = sinon.stub(); - reportError(error, subscriber, reportStub); - expect(errorStub).to.have.property('called', false); - expect(reportStub).to.have.property('called', true); + const consoleStub = sinon.stub(console, 'warn'); + try { + const error = new Error('kaboom'); + const subscriber = new Subscriber(noop); + subscriber.error(error); + const errorStub = sinon.stub(subscriber, 'error'); + reportError(error, subscriber); + expect(errorStub).to.have.property('called', false); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } }); it('should not report errors to a closed observer', () => { - const error = new Error('kaboom'); - const closed = { ...empty }; - const errorStub = sinon.stub(closed, 'error'); - const reportStub = sinon.stub(); - reportError(error, closed, reportStub); - expect(errorStub).to.have.property('called', false); - expect(reportStub).to.have.property('called', true); + const consoleStub = sinon.stub(console, 'warn'); + try { + const error = new Error('kaboom'); + const closed = { ...empty }; + const errorStub = sinon.stub(closed, 'error'); + reportError(error, closed); + expect(errorStub).to.have.property('called', false); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } }); it('should not report errors an observer with a stopped destination', () => { - const error = new Error('kaboom'); - const destination = new Subscriber(noop); - const subscriber = new Subscriber(destination); - destination.error(error); - const errorStub = sinon.stub(subscriber, 'error'); - const reportStub = sinon.stub(); - reportError(error, subscriber, reportStub); - expect(errorStub).to.have.property('called', false); - expect(reportStub).to.have.property('called', true); + const consoleStub = sinon.stub(console, 'warn'); + try { + const error = new Error('kaboom'); + const destination = new Subscriber(noop); + const subscriber = new Subscriber(destination); + destination.error(error); + const errorStub = sinon.stub(subscriber, 'error'); + reportError(error, subscriber); + expect(errorStub).to.have.property('called', false); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } }); }); diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index aaeb2740ca..14e016ef0b 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -7,11 +7,11 @@ import { ErrorObserver } from '../types'; * stopped - in which case, an alternative reporting mechanism is used. * @param err the error to report */ -export function reportError(err: any, observer: ErrorObserver, report?: (err: any) => void): void { +export function reportError(err: any, observer: ErrorObserver): void { if (canReportError(observer)) { observer.error(err); } else { - (report || consoleWarn)(err); + consoleWarn(err); } } From 3ee03c05c60056456eb4085763b337804f86b3c5 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 04:55:46 +1000 Subject: [PATCH 10/22] chore(reportError): fix whitespace --- src/internal/util/reportError.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/internal/util/reportError.ts b/src/internal/util/reportError.ts index 14e016ef0b..cf1bb839ca 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/reportError.ts @@ -9,7 +9,7 @@ import { ErrorObserver } from '../types'; */ export function reportError(err: any, observer: ErrorObserver): void { if (canReportError(observer)) { - observer.error(err); + observer.error(err); } else { consoleWarn(err); } From 9d8cb76d36be7704052649c2e4e8a285f69feb55 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 05:24:48 +1000 Subject: [PATCH 11/22] refactor(reportError): use canReportError instead --- spec/util/canReportError-spec.ts | 28 ++++++++ spec/util/reportError-spec.ts | 66 ------------------- src/internal/Observable.ts | 9 ++- .../{reportError.ts => canReportError.ts} | 20 +----- src/internal/util/consoleWarn.ts | 7 ++ 5 files changed, 44 insertions(+), 86 deletions(-) create mode 100644 spec/util/canReportError-spec.ts delete mode 100644 spec/util/reportError-spec.ts rename src/internal/util/{reportError.ts => canReportError.ts} (62%) create mode 100644 src/internal/util/consoleWarn.ts diff --git a/spec/util/canReportError-spec.ts b/spec/util/canReportError-spec.ts new file mode 100644 index 0000000000..ea96fb199b --- /dev/null +++ b/spec/util/canReportError-spec.ts @@ -0,0 +1,28 @@ +import { expect } from 'chai'; +import { noop, Subscriber } from 'rxjs'; +import { empty } from 'rxjs/internal/Observer'; +import { canReportError } from 'rxjs/internal/util/canReportError'; + +describe('reportError', () => { + it('should report errors to an observer if possible', () => { + const subscriber = new Subscriber(noop); + expect(canReportError(subscriber)).to.be.true; + }); + + it('should not report errors to a stopped observer', () => { + const subscriber = new Subscriber(noop); + subscriber.error(new Error('kaboom')); + expect(canReportError(subscriber)).to.be.false; + }); + + it('should not report errors to a closed observer', () => { + expect(canReportError(empty)).to.be.false; + }); + + it('should not report errors an observer with a stopped destination', () => { + const destination = new Subscriber(noop); + const subscriber = new Subscriber(destination); + destination.error(new Error('kaboom')); + expect(canReportError(subscriber)).to.be.false; + }); +}); diff --git a/spec/util/reportError-spec.ts b/spec/util/reportError-spec.ts deleted file mode 100644 index 43934cdcdd..0000000000 --- a/spec/util/reportError-spec.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { expect } from 'chai'; -import { noop, Subscriber } from 'rxjs'; -import { empty } from 'rxjs/internal/Observer'; -import { reportError } from 'rxjs/internal/util/reportError'; -import * as sinon from 'sinon'; - -describe('reportError', () => { - it('should report errors to an observer if possible', () => { - const consoleStub = sinon.stub(console, 'warn'); - try { - const error = new Error('kaboom'); - const subscriber = new Subscriber(noop); - const errorStub = sinon.stub(subscriber, 'error'); - reportError(error, subscriber); - expect(errorStub).to.have.property('called', true); - expect(consoleStub).to.have.property('called', false); - } finally { - consoleStub.restore(); - } - }); - - it('should not report errors to a stopped observer', () => { - const consoleStub = sinon.stub(console, 'warn'); - try { - const error = new Error('kaboom'); - const subscriber = new Subscriber(noop); - subscriber.error(error); - const errorStub = sinon.stub(subscriber, 'error'); - reportError(error, subscriber); - expect(errorStub).to.have.property('called', false); - expect(consoleStub).to.have.property('called', true); - } finally { - consoleStub.restore(); - } - }); - - it('should not report errors to a closed observer', () => { - const consoleStub = sinon.stub(console, 'warn'); - try { - const error = new Error('kaboom'); - const closed = { ...empty }; - const errorStub = sinon.stub(closed, 'error'); - reportError(error, closed); - expect(errorStub).to.have.property('called', false); - expect(consoleStub).to.have.property('called', true); - } finally { - consoleStub.restore(); - } - }); - - it('should not report errors an observer with a stopped destination', () => { - const consoleStub = sinon.stub(console, 'warn'); - try { - const error = new Error('kaboom'); - const destination = new Subscriber(noop); - const subscriber = new Subscriber(destination); - destination.error(error); - const errorStub = sinon.stub(subscriber, 'error'); - reportError(error, subscriber); - expect(errorStub).to.have.property('called', false); - expect(consoleStub).to.have.property('called', true); - } finally { - consoleStub.restore(); - } - }); -}); diff --git a/src/internal/Observable.ts b/src/internal/Observable.ts index 06ca0dd38a..b67db9d133 100644 --- a/src/internal/Observable.ts +++ b/src/internal/Observable.ts @@ -2,7 +2,8 @@ import { Operator } from './Operator'; import { Subscriber } from './Subscriber'; import { Subscription } from './Subscription'; import { TeardownLogic, OperatorFunction, PartialObserver, Subscribable } from './types'; -import { reportError } from './util/reportError'; +import { canReportError } from './util/canReportError'; +import { consoleWarn } from './util/consoleWarn'; import { toSubscriber } from './util/toSubscriber'; import { iif } from './observable/iif'; import { throwError } from './observable/throwError'; @@ -227,7 +228,11 @@ export class Observable implements Subscribable { sink.syncErrorThrown = true; sink.syncErrorValue = err; } - reportError(err, sink); + if (canReportError(sink)) { + sink.error(err); + } else { + consoleWarn(err); + } } } diff --git a/src/internal/util/reportError.ts b/src/internal/util/canReportError.ts similarity index 62% rename from src/internal/util/reportError.ts rename to src/internal/util/canReportError.ts index cf1bb839ca..eb8dba56c8 100644 --- a/src/internal/util/reportError.ts +++ b/src/internal/util/canReportError.ts @@ -7,15 +7,7 @@ import { ErrorObserver } from '../types'; * stopped - in which case, an alternative reporting mechanism is used. * @param err the error to report */ -export function reportError(err: any, observer: ErrorObserver): void { - if (canReportError(observer)) { - observer.error(err); - } else { - consoleWarn(err); - } -} - -function canReportError(observer: ErrorObserver): boolean { +export function canReportError(observer: ErrorObserver): boolean { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { return false; @@ -23,12 +15,4 @@ function canReportError(observer: ErrorObserver): boolean { return canReportError(destination); } return true; -} - -function consoleWarn(err: any): void { - if (console.warn) { - console.warn(err); - } else { - console.log(err); - } -} +} \ No newline at end of file diff --git a/src/internal/util/consoleWarn.ts b/src/internal/util/consoleWarn.ts new file mode 100644 index 0000000000..4a11866099 --- /dev/null +++ b/src/internal/util/consoleWarn.ts @@ -0,0 +1,7 @@ +export function consoleWarn(...args: any[]): void { + if (console.warn) { + console.warn(...args); + } else { + console.log(...args); + } +} From bc8f54c684071ff2dfc318fd00be6f2fc6746093 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 05:33:04 +1000 Subject: [PATCH 12/22] refactor(canReport): remove recursion --- src/internal/util/canReportError.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/internal/util/canReportError.ts b/src/internal/util/canReportError.ts index eb8dba56c8..fc287b6217 100644 --- a/src/internal/util/canReportError.ts +++ b/src/internal/util/canReportError.ts @@ -8,11 +8,15 @@ import { ErrorObserver } from '../types'; * @param err the error to report */ export function canReportError(observer: ErrorObserver): boolean { - const { closed, destination, isStopped } = observer as any; - if (closed || isStopped) { - return false; - } else if (destination instanceof Subscriber || (destination && destination[rxSubscriberSymbol])) { - return canReportError(destination); + while (observer) { + const { closed, destination, isStopped } = observer as any; + if (closed || isStopped) { + return false; + } else if (destination instanceof Subscriber || (destination && destination[rxSubscriberSymbol])) { + observer = destination; + } else { + observer = null; + } } return true; -} \ No newline at end of file +} From 1a9d49adc48179778a33bb92b503aed1697660dd Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 05:38:47 +1000 Subject: [PATCH 13/22] test(subscribe): test internal error reporting --- spec/Observable-spec.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/spec/Observable-spec.ts b/spec/Observable-spec.ts index 7e86b66d01..3627c37f15 100644 --- a/spec/Observable-spec.ts +++ b/spec/Observable-spec.ts @@ -901,5 +901,20 @@ describe('Observable.lift', () => { ]); done(); }); - }); + }); + + it('should not swallow internal errors', () => { + const consoleStub = sinon.stub(console, 'warn'); + try { + let source = new Observable(observer => observer.next(42)); + for (let i = 0; i < 10000; ++i) { + let base = source; + source = new Observable(observer => base.subscribe(observer)); + } + source.subscribe(); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } + }); }); From bbfaed00f6a47fc8deddc4171af4da2f21216d62 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 05:39:30 +1000 Subject: [PATCH 14/22] test(bindCallback): test error reporting --- spec/observables/bindCallback-spec.ts | 14 ++++++++++++++ src/internal/observable/bindCallback.ts | 8 +++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spec/observables/bindCallback-spec.ts b/spec/observables/bindCallback-spec.ts index bfe915caec..e2caf8866f 100644 --- a/spec/observables/bindCallback-spec.ts +++ b/spec/observables/bindCallback-spec.ts @@ -280,4 +280,18 @@ describe('bindCallback', () => { expect(calls).to.equal(0); }); }); + + it('should not swallow post-callback errors', () => { + function badFunction(callback: (answer: number) => void): void { + callback(42); + throw new Error('kaboom'); + } + const consoleStub = sinon.stub(console, 'warn'); + try { + bindCallback(badFunction)().subscribe(); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } + }); }); diff --git a/src/internal/observable/bindCallback.ts b/src/internal/observable/bindCallback.ts index 1ec55260ad..abdce6a32d 100644 --- a/src/internal/observable/bindCallback.ts +++ b/src/internal/observable/bindCallback.ts @@ -3,6 +3,8 @@ import { Observable } from '../Observable'; import { AsyncSubject } from '../AsyncSubject'; import { Subscriber } from '../Subscriber'; import { map } from '../operators/map'; +import { canReportError } from '../util/canReportError'; +import { consoleWarn } from '../util/consoleWarn'; import { isArray } from '../util/isArray'; import { isScheduler } from '../util/isScheduler'; @@ -204,7 +206,11 @@ export function bindCallback( try { callbackFunc.apply(context, [...args, handler]); } catch (err) { - subject.error(err); + if (canReportError(subject)) { + subject.error(err); + } else { + consoleWarn(err); + } } } return subject.subscribe(subscriber); From 1fd1807aa914897903b189360b3198470951a493 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 05:39:49 +1000 Subject: [PATCH 15/22] test(bindNodeCallback): test error reporting --- spec/observables/bindNodeCallback-spec.ts | 14 ++++++++++++++ src/internal/observable/bindNodeCallback.ts | 8 +++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/spec/observables/bindNodeCallback-spec.ts b/spec/observables/bindNodeCallback-spec.ts index 19b019b4f2..874385f57e 100644 --- a/spec/observables/bindNodeCallback-spec.ts +++ b/spec/observables/bindNodeCallback-spec.ts @@ -278,4 +278,18 @@ describe('bindNodeCallback', () => { expect(results1).to.deep.equal([42, 'done']); expect(results2).to.deep.equal([42, 'done']); }); + + it('should not swallow post-callback errors', () => { + function badFunction(callback: (error: Error, answer: number) => void): void { + callback(null, 42); + throw new Error('kaboom'); + } + const consoleStub = sinon.stub(console, 'warn'); + try { + bindNodeCallback(badFunction)().subscribe(); + expect(consoleStub).to.have.property('called', true); + } finally { + consoleStub.restore(); + } + }); }); diff --git a/src/internal/observable/bindNodeCallback.ts b/src/internal/observable/bindNodeCallback.ts index 4aefc2f755..5896687f27 100644 --- a/src/internal/observable/bindNodeCallback.ts +++ b/src/internal/observable/bindNodeCallback.ts @@ -3,6 +3,8 @@ import { AsyncSubject } from '../AsyncSubject'; import { Subscriber } from '../Subscriber'; import { SchedulerAction, SchedulerLike } from '../types'; import { map } from '../operators/map'; +import { canReportError } from '../util/canReportError'; +import { consoleWarn } from '../util/consoleWarn'; import { isScheduler } from '../util/isScheduler'; import { isArray } from '../util/isArray'; @@ -198,7 +200,11 @@ export function bindNodeCallback( try { callbackFunc.apply(context, [...args, handler]); } catch (err) { - subject.error(err); + if (canReportError(subject)) { + subject.error(err); + } else { + consoleWarn(err); + } } } return subject.subscribe(subscriber); From 6f404129347fa81e294380696493b1ac836f4d48 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 06:05:13 +1000 Subject: [PATCH 16/22] chore(canReport): fix JSDoc --- src/internal/util/canReportError.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/internal/util/canReportError.ts b/src/internal/util/canReportError.ts index fc287b6217..479b9f02de 100644 --- a/src/internal/util/canReportError.ts +++ b/src/internal/util/canReportError.ts @@ -3,9 +3,10 @@ import { rxSubscriber as rxSubscriberSymbol } from '../symbol/rxSubscriber'; import { ErrorObserver } from '../types'; /** - * Reports the error to the ErrorObserver unless the observer is closed or - * stopped - in which case, an alternative reporting mechanism is used. - * @param err the error to report + * Determines whether the ErrorObserver is closed or stopped or has a + * destination that is closed or stopped - in which case errors will + * need to be reported via a different mechanism. + * @param observer the observer */ export function canReportError(observer: ErrorObserver): boolean { while (observer) { From daa02c43e2e996ba49a3e38234345440e8790ed3 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 10:23:09 +1000 Subject: [PATCH 17/22] chore(test): fix test description --- spec/util/canReportError-spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/util/canReportError-spec.ts b/spec/util/canReportError-spec.ts index ea96fb199b..10b3f0e878 100644 --- a/spec/util/canReportError-spec.ts +++ b/spec/util/canReportError-spec.ts @@ -3,7 +3,7 @@ import { noop, Subscriber } from 'rxjs'; import { empty } from 'rxjs/internal/Observer'; import { canReportError } from 'rxjs/internal/util/canReportError'; -describe('reportError', () => { +describe('canReportError', () => { it('should report errors to an observer if possible', () => { const subscriber = new Subscriber(noop); expect(canReportError(subscriber)).to.be.true; From ed2a61762c837537843bd92217d3810c1a98bfd1 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Wed, 5 Sep 2018 10:43:29 +1000 Subject: [PATCH 18/22] test(canReport): use noop error handlers --- spec/util/canReportError-spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/util/canReportError-spec.ts b/spec/util/canReportError-spec.ts index 10b3f0e878..045193f576 100644 --- a/spec/util/canReportError-spec.ts +++ b/spec/util/canReportError-spec.ts @@ -5,12 +5,12 @@ import { canReportError } from 'rxjs/internal/util/canReportError'; describe('canReportError', () => { it('should report errors to an observer if possible', () => { - const subscriber = new Subscriber(noop); + const subscriber = new Subscriber(noop, noop); expect(canReportError(subscriber)).to.be.true; }); it('should not report errors to a stopped observer', () => { - const subscriber = new Subscriber(noop); + const subscriber = new Subscriber(noop, noop); subscriber.error(new Error('kaboom')); expect(canReportError(subscriber)).to.be.false; }); @@ -20,7 +20,7 @@ describe('canReportError', () => { }); it('should not report errors an observer with a stopped destination', () => { - const destination = new Subscriber(noop); + const destination = new Subscriber(noop, noop); const subscriber = new Subscriber(destination); destination.error(new Error('kaboom')); expect(canReportError(subscriber)).to.be.false; From 1b53211864d935a55dceb880d4688351c892dd65 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sat, 8 Sep 2018 06:58:56 +1000 Subject: [PATCH 19/22] chore(canReport): use isTrustedSubscriber --- src/internal/Subscriber.ts | 2 +- src/internal/util/canReportError.ts | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index d7dd3c84de..7f81c5814b 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -325,6 +325,6 @@ export class SafeSubscriber extends Subscriber { } } -function isTrustedSubscriber(obj: any) { +export function isTrustedSubscriber(obj: any) { return obj instanceof Subscriber || ('_addParentTeardownLogic' in obj && obj[rxSubscriberSymbol]); } diff --git a/src/internal/util/canReportError.ts b/src/internal/util/canReportError.ts index 479b9f02de..f66012e70c 100644 --- a/src/internal/util/canReportError.ts +++ b/src/internal/util/canReportError.ts @@ -1,5 +1,4 @@ -import { Subscriber } from '../Subscriber'; -import { rxSubscriber as rxSubscriberSymbol } from '../symbol/rxSubscriber'; +import { isTrustedSubscriber } from '../Subscriber'; import { ErrorObserver } from '../types'; /** @@ -13,7 +12,7 @@ export function canReportError(observer: ErrorObserver): boolean { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { return false; - } else if (destination instanceof Subscriber || (destination && destination[rxSubscriberSymbol])) { + } else if (destination && isTrustedSubscriber(destination)) { observer = destination; } else { observer = null; From 30440d2e4514350d24d80aa37ec669c669fb5f0b Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sat, 8 Sep 2018 07:01:54 +1000 Subject: [PATCH 20/22] chore(canReport): restrict observer type --- src/internal/util/canReportError.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/internal/util/canReportError.ts b/src/internal/util/canReportError.ts index f66012e70c..caa98f7ea6 100644 --- a/src/internal/util/canReportError.ts +++ b/src/internal/util/canReportError.ts @@ -1,5 +1,5 @@ -import { isTrustedSubscriber } from '../Subscriber'; -import { ErrorObserver } from '../types'; +import { isTrustedSubscriber, Subscriber } from '../Subscriber'; +import { Subject } from '../Subject'; /** * Determines whether the ErrorObserver is closed or stopped or has a @@ -7,7 +7,7 @@ import { ErrorObserver } from '../types'; * need to be reported via a different mechanism. * @param observer the observer */ -export function canReportError(observer: ErrorObserver): boolean { +export function canReportError(observer: Subscriber | Subject): boolean { while (observer) { const { closed, destination, isStopped } = observer as any; if (closed || isStopped) { From d640e9deb8b5eb12659108fa6278b1c6cf60fec9 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sat, 8 Sep 2018 07:41:33 +1000 Subject: [PATCH 21/22] chore(canReport): fix tests --- spec/util/canReportError-spec.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/util/canReportError-spec.ts b/spec/util/canReportError-spec.ts index 045193f576..77df00d2ae 100644 --- a/spec/util/canReportError-spec.ts +++ b/spec/util/canReportError-spec.ts @@ -1,27 +1,28 @@ import { expect } from 'chai'; -import { noop, Subscriber } from 'rxjs'; -import { empty } from 'rxjs/internal/Observer'; +import { noop, Subject, Subscriber } from 'rxjs'; import { canReportError } from 'rxjs/internal/util/canReportError'; describe('canReportError', () => { it('should report errors to an observer if possible', () => { - const subscriber = new Subscriber(noop, noop); + const subscriber = new Subscriber<{}>(noop, noop); expect(canReportError(subscriber)).to.be.true; }); it('should not report errors to a stopped observer', () => { - const subscriber = new Subscriber(noop, noop); + const subscriber = new Subscriber<{}>(noop, noop); subscriber.error(new Error('kaboom')); expect(canReportError(subscriber)).to.be.false; }); - it('should not report errors to a closed observer', () => { - expect(canReportError(empty)).to.be.false; + it('should not report errors to a closed subject', () => { + const subject = new Subject<{}>(); + subject.unsubscribe(); + expect(canReportError(subject)).to.be.false; }); it('should not report errors an observer with a stopped destination', () => { - const destination = new Subscriber(noop, noop); - const subscriber = new Subscriber(destination); + const destination = new Subscriber<{}>(noop, noop); + const subscriber = new Subscriber<{}>(destination); destination.error(new Error('kaboom')); expect(canReportError(subscriber)).to.be.false; }); From 14a6c61d39359f469941b8c3e6dce9c8540504d4 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sat, 8 Sep 2018 10:00:32 +1000 Subject: [PATCH 22/22] chore(canReport): use console.warn directly --- src/internal/Observable.ts | 3 +-- src/internal/observable/bindCallback.ts | 3 +-- src/internal/observable/bindNodeCallback.ts | 3 +-- src/internal/util/consoleWarn.ts | 7 ------- 4 files changed, 3 insertions(+), 13 deletions(-) delete mode 100644 src/internal/util/consoleWarn.ts diff --git a/src/internal/Observable.ts b/src/internal/Observable.ts index b67db9d133..78b7506190 100644 --- a/src/internal/Observable.ts +++ b/src/internal/Observable.ts @@ -3,7 +3,6 @@ import { Subscriber } from './Subscriber'; import { Subscription } from './Subscription'; import { TeardownLogic, OperatorFunction, PartialObserver, Subscribable } from './types'; import { canReportError } from './util/canReportError'; -import { consoleWarn } from './util/consoleWarn'; import { toSubscriber } from './util/toSubscriber'; import { iif } from './observable/iif'; import { throwError } from './observable/throwError'; @@ -231,7 +230,7 @@ export class Observable implements Subscribable { if (canReportError(sink)) { sink.error(err); } else { - consoleWarn(err); + console.warn(err); } } } diff --git a/src/internal/observable/bindCallback.ts b/src/internal/observable/bindCallback.ts index abdce6a32d..3166a41fa2 100644 --- a/src/internal/observable/bindCallback.ts +++ b/src/internal/observable/bindCallback.ts @@ -4,7 +4,6 @@ import { AsyncSubject } from '../AsyncSubject'; import { Subscriber } from '../Subscriber'; import { map } from '../operators/map'; import { canReportError } from '../util/canReportError'; -import { consoleWarn } from '../util/consoleWarn'; import { isArray } from '../util/isArray'; import { isScheduler } from '../util/isScheduler'; @@ -209,7 +208,7 @@ export function bindCallback( if (canReportError(subject)) { subject.error(err); } else { - consoleWarn(err); + console.warn(err); } } } diff --git a/src/internal/observable/bindNodeCallback.ts b/src/internal/observable/bindNodeCallback.ts index 5896687f27..d40e4fb6a4 100644 --- a/src/internal/observable/bindNodeCallback.ts +++ b/src/internal/observable/bindNodeCallback.ts @@ -4,7 +4,6 @@ import { Subscriber } from '../Subscriber'; import { SchedulerAction, SchedulerLike } from '../types'; import { map } from '../operators/map'; import { canReportError } from '../util/canReportError'; -import { consoleWarn } from '../util/consoleWarn'; import { isScheduler } from '../util/isScheduler'; import { isArray } from '../util/isArray'; @@ -203,7 +202,7 @@ export function bindNodeCallback( if (canReportError(subject)) { subject.error(err); } else { - consoleWarn(err); + console.warn(err); } } } diff --git a/src/internal/util/consoleWarn.ts b/src/internal/util/consoleWarn.ts deleted file mode 100644 index 4a11866099..0000000000 --- a/src/internal/util/consoleWarn.ts +++ /dev/null @@ -1,7 +0,0 @@ -export function consoleWarn(...args: any[]): void { - if (console.warn) { - console.warn(...args); - } else { - console.log(...args); - } -}