fix(groupBy): Fix groupBy to dispose of outer subscription.

[trxcllnt]

Not sure how groupBy is passing current unit tests, as we seem to be covering this case.

This PR fixes a bug where groupBy wasn't unsubscribing from its source when the outer subscription has been disposed and all inner subscriptions are inactive.

If you see here, the inner refCount subscription calls parent.unsubscribe() if parent.count === 0 and parent.attemptedToUnsubscribe is true. But the parent only unsubscribes if parent.attemptedToUnsubscribe is false, which means the subscription to the source is never disposed.

This is easily replicated with groupBy on a Subject, where you'll see the observers list grow indefinitely.

[coveralls]

Coverage remained the same at 97.674% when pulling 04b7757 on trxcllnt:fix-groupBy-unsubscribe into dc06e01 on ReactiveX:5.0.x.

[kwonoj]

So we can't create create test coverage for this kind of cases?

[trxcllnt]

@kwonoj I'm not sure. This test looks to be covering this case. I tried making a test that broke without this patch applied, but couldn't get it to break outside of the production code I had. It's possible my production code synchronously unsubscribes, which would explain why I can't replicate with marble diagrams (yet).

[trxcllnt]

@kwonoj if you run this example, you can see the interval subscription isn't being disposed:

import { Observable } from 'rxjs';

console.clear();

Observable
  .interval(100)
  .do(console.log.bind(console, 'interval'))
  .finally(console.log.bind(console, 'unsubscribed'))
  .take(10)
  .mapTo(1)
  .groupBy((x) => x)
  .take(1)
  .mergeMap((group) => group.take(1))
  .subscribe(
    console.log.bind(console, 'next'),
    console.log.bind(console, 'error'),
    console.log.bind(console, 'complete')
  );
/*
> Console was cleared
> interval 0
> next 1
> complete undefined
> interval 1
> interval 2
> interval 3
> interval 4
> interval 5
> interval 6
> interval 7
> interval 8
> interval 9
> unsubscribed
*/

[benlesh]

@trxcllnt can we definitively add a failing test case for this? I'd like to see that before we can merge this.

[trxcllnt]

@Blesh ok, I found a way to add a marble test that fails without this patch applied. the PR has been updated with this test. This is a critical memory leak in our codebase (and I imagine, many others), so if we could fast track this merge and cut a 5.0.2 that'd be great.

[coveralls]

Coverage remained the same at 97.674% when pulling 6ea5b17 on trxcllnt:fix-groupBy-unsubscribe into dc06e01 on ReactiveX:5.0.x.

[mattpodwysocki]

LGTM

[trxcllnt]

@Blesh did this get merged??

[benlesh]

Hmmm... @trxcllnt I think this got merged into 5.0.x, which was deleted 😱. It's all good, I think we can merge this into master.

[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.