Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should groupBy keep connections after the result subscription is unsubscribed? #6805

Closed
benlesh opened this issue Feb 7, 2022 · 4 comments · Fixed by #7252
Closed

Should groupBy keep connections after the result subscription is unsubscribed? #6805

benlesh opened this issue Feb 7, 2022 · 4 comments · Fixed by #7252
Assignees
Labels
8.x Issues and PRs for version 8.x docs Issues and PRs related to documentation

Comments

@benlesh
Copy link
Member

benlesh commented Feb 7, 2022

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 benlesh added 8.x Issues and PRs for version 8.x AGENDA ITEM Flagged for discussion at core team meetings labels Feb 7, 2022
benlesh added a commit to benlesh/rxjs that referenced this issue Feb 7, 2022
Adds a special case to `OperatorSubscriber` to support `groupBy`.

Related: ReactiveX#6805 ReactiveX#6803 ReactiveX#6804
benlesh added a commit that referenced this issue Feb 8, 2022
Adds a special case to `OperatorSubscriber` to support `groupBy`.

Related: #6805 #6803 #6804
@benlesh
Copy link
Member Author

benlesh commented Feb 23, 2022

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

@benlesh
Copy link
Member Author

benlesh commented Mar 9, 2022

Core Team: We have quorum! Kill it!

@benlesh benlesh self-assigned this Mar 9, 2022
@backbone87
Copy link
Contributor

ref #6434 (comment)

@benlesh
Copy link
Member Author

benlesh commented Aug 24, 2022

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 benlesh added docs Issues and PRs related to documentation and removed AGENDA ITEM Flagged for discussion at core team meetings labels Aug 24, 2022
benlesh added a commit to benlesh/rxjs that referenced this issue Apr 17, 2023
`groupBy` no longer supports the behavior where inner subscriptions will cause the outer subscription to stay connected after consumer unsubscribes from result.

Resolves ReactiveX#6805

BREAKING CHANGE: `groupBy` no longer allows grouped observable subscriptions to stay connected after parent subscription unsubscribed. If you need this behavior, don't unsubscribe from the parent.
benlesh added a commit that referenced this issue May 15, 2023
* fix(groupBy): will no longer leak inner subscriptions

`groupBy` no longer supports the behavior where inner subscriptions will cause the outer subscription to stay connected after consumer unsubscribes from result.

Resolves #6805

BREAKING CHANGE: `groupBy` no longer allows grouped observable subscriptions to stay connected after parent subscription unsubscribed. If you need this behavior, don't unsubscribe from the parent.

* refactor: Remove direct instantiation of `OperatorSubscriber`

+ Stopped exporting `OperatorSubscriber` class from module
+ Updated `onErrorResumeNext` and a `Subject` test to use `createOperatorSubscriber`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Issues and PRs for version 8.x docs Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants