fix: Ensure unsubscriptions/teardowns on internal subscribers are idempotent (#5465)

* null out _unsubscribe after unsubscription

Fixes #5464

* test: add idempotent unsubscribe tests

* fix: null the subscription ctor teardown

* refactor: use delete to remove the member

* refactor: delete _unsubscribe only if it exists

* refactor: replace delete with hasOwnProperty etc

* refactor: use _ctorUnsubscribe flag

Co-authored-by: Nicholas Jamieson <[email protected]>

Author: 2 people

spec/Subscriber-spec.ts

@@ -97,4 +97,17 @@ describe('Subscriber', () => {
 
     expect(argument).to.have.lengthOf(0);
   });
+
+  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);
+  });
 });

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);
+    });
   });
 });

src/internal/Subscription.ts

@@ -40,7 +40,8 @@ export class Subscription implements SubscriptionLike {
    */
   constructor(unsubscribe?: () => void) {
     if (unsubscribe) {
-      (<any> this)._unsubscribe = unsubscribe;
+      (this as any)._ctorUnsubscribe = true;
+      (this as any)._unsubscribe = unsubscribe;
     }
   }
 
@@ -57,7 +58,7 @@ export class Subscription implements SubscriptionLike {
       return;
     }
 
-    let { _parentOrParents, _unsubscribe, _subscriptions } = (<any> this);
+    let { _parentOrParents, _ctorUnsubscribe, _unsubscribe, _subscriptions } = (this as any);
 
     this.closed = true;
     this._parentOrParents = null;
@@ -75,6 +76,18 @@ 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 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 {
         _unsubscribe.call(this);
       } catch (e) {
@@ -131,7 +144,7 @@ export class Subscription implements SubscriptionLike {
   add(teardown: TeardownLogic): Subscription {
     let subscription = (<Subscription>teardown);
 
-    if (!(<any>teardown)) {
+    if (!teardown) {
       return Subscription.EMPTY;
     }