From d92c97b583c0a183620cf3933436113a1661e987 Mon Sep 17 00:00:00 2001 From: Felix Becker Date: Mon, 1 Jun 2020 13:45:49 +0200 Subject: [PATCH 1/7] null out _unsubscribe after unsubscription Fixes #5464 --- src/internal/Subscription.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 8d4b80f24e..40bfc8988c 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -63,6 +63,7 @@ export class Subscription implements SubscriptionLike { // null out _subscriptions first so any child subscriptions that attempt // to remove themselves from this subscription will noop this._subscriptions = null; + ( this)._unsubscribe = null; if (_parentOrParents instanceof Subscription) { _parentOrParents.remove(this); From 4022017ad2bdde87d2933d2ecd649d60381b7415 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 2 Jun 2020 08:36:55 +1000 Subject: [PATCH 2/7] test: add idempotent unsubscribe tests --- spec/Subscriber-spec.ts | 13 +++++++++++++ spec/Subscription-spec.ts | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/spec/Subscriber-spec.ts b/spec/Subscriber-spec.ts index ab5efae64d..374d03d559 100644 --- a/spec/Subscriber-spec.ts +++ b/spec/Subscriber-spec.ts @@ -116,4 +116,17 @@ describe('Subscriber', () => { expect(subscriberUnsubscribed).to.be.true; expect(subscriptionUnsubscribed).to.be.true; }); + + it('should have idempotent unsubscription', () => { + let count = 0; + const subscriber = new Subscriber(); + subscriber.add(() => ++count); + expect(count).to.equal(0); + + subscriber.unsubscribe(); + expect(count).to.equal(1); + + subscriber.unsubscribe(); + expect(count).to.equal(1); + }); }); diff --git a/spec/Subscription-spec.ts b/spec/Subscription-spec.ts index 21d769f666..2544e57751 100644 --- a/spec/Subscription-spec.ts +++ b/spec/Subscription-spec.ts @@ -159,5 +159,17 @@ describe('Subscription', () => { done(); }); }); + + it('Should have idempotent unsubscription', () => { + let count = 0; + const subscription = new Subscription(() => ++count); + expect(count).to.equal(0); + + subscription.unsubscribe(); + expect(count).to.equal(1); + + subscription.unsubscribe(); + expect(count).to.equal(1); + }); }); }); From a07b74f3e5d14ca195f300eb5b75665df5f1fd16 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Tue, 2 Jun 2020 09:03:03 +1000 Subject: [PATCH 3/7] fix: null the subscription ctor teardown --- src/internal/Subscription.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 40bfc8988c..134b25486b 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -39,7 +39,16 @@ export class Subscription implements SubscriptionLike { */ constructor(unsubscribe?: () => void) { if (unsubscribe) { - ( this)._unsubscribe = unsubscribe; + // Wrap the unsubscribe teardown in a function so that the argument can + // be nulled. It's not possible to null the _unsubscribe member as there + // are many classes that are derived from Subscriber (which derives from + // Subscription) that implement an _unsubscribe method as a mechanism for + // obtaining unsubscription notifications and some of those subscribers + // are recycled. + ( this)._unsubscribe = () => { + unsubscribe!(); + unsubscribe = undefined; + }; } } @@ -63,7 +72,6 @@ export class Subscription implements SubscriptionLike { // null out _subscriptions first so any child subscriptions that attempt // to remove themselves from this subscription will noop this._subscriptions = null; - ( this)._unsubscribe = null; if (_parentOrParents instanceof Subscription) { _parentOrParents.remove(this); From 34a513ba1ff99b41467bc5f6a1cfcbdfcff54b71 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Thu, 4 Jun 2020 09:25:02 +1000 Subject: [PATCH 4/7] refactor: use delete to remove the member --- src/internal/Subscription.ts | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 134b25486b..42ea063163 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -39,16 +39,7 @@ export class Subscription implements SubscriptionLike { */ constructor(unsubscribe?: () => void) { if (unsubscribe) { - // Wrap the unsubscribe teardown in a function so that the argument can - // be nulled. It's not possible to null the _unsubscribe member as there - // are many classes that are derived from Subscriber (which derives from - // Subscription) that implement an _unsubscribe method as a mechanism for - // obtaining unsubscription notifications and some of those subscribers - // are recycled. - ( this)._unsubscribe = () => { - unsubscribe!(); - unsubscribe = undefined; - }; + (this as any)._unsubscribe = unsubscribe; } } @@ -65,13 +56,20 @@ export class Subscription implements SubscriptionLike { return; } - let { _parentOrParents, _unsubscribe, _subscriptions } = ( this); + let { _parentOrParents, _unsubscribe, _subscriptions } = (this as any); this.closed = true; this._parentOrParents = null; // null out _subscriptions first so any child subscriptions that attempt // to remove themselves from this subscription will noop this._subscriptions = null; + // It's not possible to null the _unsubscribe member as there are many + // classes that are derived from Subscriber (which derives from + // Subscription) that implement an _unsubscribe method as a mechanism for + // obtaining unsubscription notifications and some of those subscribers are + // recycled. Deleting the member will release the reference to any teardown + // functions passed in the constructor and will leave any methods intact. + delete (this as any)._unsubscribe; if (_parentOrParents instanceof Subscription) { _parentOrParents.remove(this); @@ -139,7 +137,7 @@ export class Subscription implements SubscriptionLike { add(teardown: TeardownLogic): Subscription { let subscription = (teardown); - if (!(teardown)) { + if (!teardown) { return Subscription.EMPTY; } From 125a410442d5052c8af10dc76adeac320bb72a42 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Thu, 4 Jun 2020 09:50:20 +1000 Subject: [PATCH 5/7] refactor: delete _unsubscribe only if it exists --- src/internal/Subscription.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 42ea063163..b923ca345a 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -63,13 +63,6 @@ export class Subscription implements SubscriptionLike { // null out _subscriptions first so any child subscriptions that attempt // to remove themselves from this subscription will noop this._subscriptions = null; - // It's not possible to null the _unsubscribe member as there are many - // classes that are derived from Subscriber (which derives from - // Subscription) that implement an _unsubscribe method as a mechanism for - // obtaining unsubscription notifications and some of those subscribers are - // recycled. Deleting the member will release the reference to any teardown - // functions passed in the constructor and will leave any methods intact. - delete (this as any)._unsubscribe; if (_parentOrParents instanceof Subscription) { _parentOrParents.remove(this); @@ -81,6 +74,14 @@ export class Subscription implements SubscriptionLike { } if (isFunction(_unsubscribe)) { + // It's not possible to null the _unsubscribe member as there are many + // classes that are derived from Subscriber (which derives from + // Subscription) that implement an _unsubscribe method as a mechanism for + // obtaining unsubscription notifications and some of those subscribers + // are recycled. Deleting the member will release the reference to any + // teardown functions passed in the constructor and will leave any + // methods intact. + delete (this as any)._unsubscribe; try { _unsubscribe.call(this); } catch (e) { From 6b61b384ea3d441fc025eb811445a23c4048e0ac Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Fri, 3 Jul 2020 16:32:13 +1000 Subject: [PATCH 6/7] refactor: replace delete with hasOwnProperty etc --- src/internal/Subscription.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index b923ca345a..0ce94f6c5e 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -74,14 +74,15 @@ export class Subscription implements SubscriptionLike { } if (isFunction(_unsubscribe)) { - // It's not possible to null the _unsubscribe member as there are many - // classes that are derived from Subscriber (which derives from - // Subscription) that implement an _unsubscribe method as a mechanism for - // obtaining unsubscription notifications and some of those subscribers - // are recycled. Deleting the member will release the reference to any - // teardown functions passed in the constructor and will leave any - // methods intact. - delete (this as any)._unsubscribe; + // It's only possible to null _unsubscribe - to release the reference to + // any teardown function passed in the constructor - if it is actually an + // instance property, as there are some classes that are derived from + // Subscriber (which derives from Subscription) that implement an + // _unsubscribe method as a mechanism for obtaining unsubscription + // notifications and some of those subscribers are recycled. + if (this.hasOwnProperty('_unsubscribe')) { + (this as any)._unsubscribe = undefined; + } try { _unsubscribe.call(this); } catch (e) { From 3ef0e1f1cf532e4fc9b7ab94a394e884f71b724c Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 26 Jul 2020 11:46:01 +1000 Subject: [PATCH 7/7] refactor: use _ctorUnsubscribe flag --- src/internal/Subscription.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 0ce94f6c5e..646745ec43 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -39,6 +39,7 @@ export class Subscription implements SubscriptionLike { */ constructor(unsubscribe?: () => void) { if (unsubscribe) { + (this as any)._ctorUnsubscribe = true; (this as any)._unsubscribe = unsubscribe; } } @@ -56,7 +57,7 @@ export class Subscription implements SubscriptionLike { return; } - let { _parentOrParents, _unsubscribe, _subscriptions } = (this as any); + let { _parentOrParents, _ctorUnsubscribe, _unsubscribe, _subscriptions } = (this as any); this.closed = true; this._parentOrParents = null; @@ -75,12 +76,15 @@ export class Subscription implements SubscriptionLike { if (isFunction(_unsubscribe)) { // It's only possible to null _unsubscribe - to release the reference to - // any teardown function passed in the constructor - if it is actually an - // instance property, as there are some classes that are derived from - // Subscriber (which derives from Subscription) that implement an - // _unsubscribe method as a mechanism for obtaining unsubscription - // notifications and some of those subscribers are recycled. - if (this.hasOwnProperty('_unsubscribe')) { + // any teardown function passed in the constructor - if the property was + // actually assigned in the constructor, as there are some classes that + // are derived from Subscriber (which derives from Subscription) that + // implement an _unsubscribe method as a mechanism for obtaining + // unsubscription notifications and some of those subscribers are + // recycled. Also, in some of those subscribers, _unsubscribe switches + // from a prototype method to an instance property - see notifyNext in + // RetryWhenSubscriber. + if (_ctorUnsubscribe) { (this as any)._unsubscribe = undefined; } try {