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

GroupBy subscriptions not disposed in due time - durationSelector #2660

Closed
crunchie84 opened this issue Jun 13, 2017 · 2 comments
Closed

GroupBy subscriptions not disposed in due time - durationSelector #2660

crunchie84 opened this issue Jun 13, 2017 · 2 comments

Comments

@crunchie84
Copy link
Contributor

RxJS version:
5.4.0
Code to reproduce:

const ticker = Rx.Observable.interval(100)
  .map(tick => ({
    key: tick
  }))
;


ticker
  .groupBy(
    keySelector => keySelector.key % 2,
    null,
    durationSelector => ticker
      .filter(tick => tick.key > durationSelector.key + 10)
      .do(val => console.log(`[durationSelector] emitting value for group ${durationSelector.key}: ${val.key}`), null, () => console.log(`[durationSelector] completed for group ${durationSelector.key}`))
      .finally(() => console.log(`[durationSelector] unsubcribed for group ${durationSelector.key}`))
//    .take(1) // <--THIS FIXES IT
  )
  .take(3)
  .subscribe(
    val => console.log(`grouping: ${val.key}`),
err => console.error(err),
() => console.log('completed'));

Live version can be found at t his jsbin https://jsbin.com/vilacos/16/edit?js,console

Expected behavior:

When the durationSelector emits a value the group is completed. This should make the GroupBySubscriber also unsubscribe from the durationSelector so it can be garbage collected.

We would expect the finally() to be invoked directly after the durationSelector has emitted a value because it completes the groupBy grouping thus creating the following console logs:

grouping: 0
grouping: 1
[durationSelector] emitting value for group 0: 11
[durationSelector] unsubcribed for group 0

but instead the unsubscribe from the durationSelectors are only emitted when the groupBy completes due to the take(3) in the outer stream.

Appending .take(1) to the durationSelector stream manually fixes this behaviour (note; this was implemented as such in rxjs4 groupbyuntil.)

Actual behavior:

The durationSelector is not unsubscribed when the group has been completed. Only when the GroupBy is completed itself it releases all its captured durationSelectors. This results in hoarding of memory until OOM (our case) or parent stream is completed.

@crunchie84 crunchie84 changed the title GroupBy subscription leak - durationSelector GroupBy subscriptions not disposed in due time - durationSelector Jun 13, 2017
hermanbanken added a commit to hermanbanken/RxJS that referenced this issue Jun 13, 2017
…e group

The Groups are disposed by the GroupDurationSelector, however the
GroupDurationSubscriber can be subscribed to a different observable
than the group itself. To prevent any unwanted subscriptions to
accumulate over time we need to explicitly unsubscribe after the
first event in GroupDurationSubscriber closes the group.

Fixes ReactiveX#2660
@hermanbanken
Copy link
Contributor

I added a test to verify the observables returned by the durationSelector where indeed not closed in 4bd3ea2. A simple fix is something like 1a03cc2.

@lock
Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants