ChildSubscriptions unsubscribed but not removed.

[johncvrs]

In the switch() operator, you'll find this code used to add
subscriptions to the Subscriptions._subscriptions[] array:

this.add(this.innerSubscription = subscribeToResult(this, value));

And you'll find this code to remove them:

const innerSubscription = this.innerSubscription;
if (innerSubscription) {
  innerSubscription.unsubscribe();
  this.remove(innerSubscription);
}

But note behavior of Subscription.add(s) is not "add s to the array and return s";
behavior is "create a new ChildSubscription from s, add the ChildSubscription to the array,
and return the ChildSubscription."
The consequence? The remove() does not match the add(). The remove() becomes a silent no-op.
The unsubscribe() was successful (good - we get correct program behavior)
but the remove() fails (bad - cannot GC these silent ChildSubscriptions).

RxJS version:
v5.1.0

Code to reproduce:

var parentTicker = Rx.Observable.interval(1000);
var childTickers = parentTicker.map((x) => {
  return Rx.Observable.interval(100).take(2);
});

var switcher = childTickers.switch();
var sub = switcher.subscribe(function(x){
        console.log('tick #' + x + ': subscriptions = ' +
          sub._subscriptions[0]._innerSub._subscriptions.length);
      });

http://jsbin.com/zotobo/edit?js,console

Additional information:

PRs with specs and fixes for two operators (switch(), throttle())
will follow this message. Take a look and tell me if you agree
that this is a legit problem and not failed understanding on my part.

( Because if you do agree, this problem also exists in many
operators. Operators audit(), switchMap(), switchMapTo(), windowToggle(),
windowWhen() can be fixed as above, but fixing MergeMap() e.g., is going to
require stepping back and deciding if remove() responsibility should
even live in the operators, or if it should live in the Subscription class
or ChildSubscription class rolled into the unsubscribe methods. )

[johncvrs]

Oh, should have included the fix in this message, not just the PR. For the switch and throttle operators the fix is simply:

Before

this.add(this.innerSubscription = subscribeToResult(this, value));

After

this.innerSubscription = this.add(subscribeToResult(this, value));

And with this in place, this line

this.remove(innerSubscription);

is not even needed (ChildSubscriptions remove themselves on unsubscribe).

For other operators, fixing is not so simple.

[kwonoj]

coincidencely I noticed numbers of childsubscription grows up with switchmap yesterday, seems this issue is culprit (as noted in issue itself), but haven't digged myself into yet.

[trxcllnt]

Yes this seems serious, and no doubt affects many operators. @Blesh perhaps there's a surgical fix we can make to Subscription that would solve this everywhere?

[trxcllnt]

@johncvrs after auditing the operator impls, and issue #2244 that led to introducing ChildSubscription, I think we can make a surgical fix to Subscription that will solve this problem comprehensively. Going to try to get that PR done asap, thanks for reporting and submitting your PR.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.