rxjs icon indicating copy to clipboard operation
rxjs copied to clipboard

Should `groupBy` keep connections after the result subscription is unsubscribed?

Open benlesh opened this issue 3 years ago • 4 comments

We currently have special case handling for groupBy that purposefully introduces the following seemingly non-deterministic behavior:

Stackblitz

import { tap, interval, groupBy } from 'rxjs';

const subscription = interval(100)
  .pipe(
    groupBy((n) => (n % 2 === 0 ? 'even' : 'odd')),
    tap((grouped$) => {
      grouped$.subscribe((n) => console.log(grouped$.key, n));
    })
  )
  .subscribe();

// Toggle to true or false to try different scenarios.
const unsubImmediately = true;

// Unsubscribe here and it stops! (no groups have been recieved yet).
// Result subscription was unsubscribed before any groups could be subscribed to.
if (unsubImmediately) {
  subscription.unsubscribe();
} else {
  setTimeout(() => {
    // Here the source subscription goes on forever!!
    // We have 2 groups, and someone's subscribed to them in a way they're not unsubscribed
    // when the result subscription is.
    subscription.unsubscribe();
  }, 1000);
}

Basically, if anything subscribes to a grouped observable and doesn't unsubscribe, unsubscribing from the outer subscription will not unsubscribe from the source. If there are no subscriptions to the grouped observables, then the subscription to the source will be unsubscribed.

What is the issue?

This is what groupBy has been doing since long before I came onto the project. However, my gut tells me this behavior is inconsistent with the rest of the framework, saving the default behavior of shareReplay(), which has itself caused a lot of confusion. The behavior, as demonstrated by the Stackblitz above, can be hard to reason about if event sources or subscription management isn't deterministic.

This behavior is such a one-off thing it's wildly unlike any other operator we export.

Proposed change

I feel that outer subscriptions to the resulting observable returned by groupBy should unsubscribe/disconnect the source from the groups when unsubscribed. In this case, the grouped observables, if still subscribed, could either: A) unsubscribe on their own, or B) emit an error saying they were disconnected. I feel like A is probably the proper solution, but we could even make that configurable.

(This would be a BREAKING change for groupBy, in the context that grouped observables would shut down if the source is disconnected)

benlesh avatar Feb 07 '22 23:02 benlesh

Quick thoughts, no quorum: Deprecate it (@kwonoj), kill it with fire (@benlesh)

benlesh avatar Feb 23 '22 21:02 benlesh

Core Team: We have quorum! Kill it!

benlesh avatar Mar 09 '22 21:03 benlesh

ref https://github.com/ReactiveX/rxjs/pull/6434#discussion_r641849113

backbone87 avatar Apr 10 '22 20:04 backbone87

Core Team Round 2: Still Quorum.

Also:

  1. Add documentation to groupBy warning people about this weirdness.
  2. There should be a lint rule, @cartant! This is your fault! (We love you anyway)

benlesh avatar Aug 24 '22 20:08 benlesh