Memory Leak with Queue Scheduler and Subject #2244

MikeRyanDev · 2017-01-02T20:45:18Z

RxJS version: 5.0.2

Code to reproduce:
Observing a subject using the queue scheduler creates many QueueActions that are never freed.

Plnkr demonstrating issue: https://plnkr.co/edit/BBkHlpIf6MiP2eOPdIlz

Minimal code to reproduce:

import 'rxjs/add/operator/observeOn';
import { interval } from 'rxjs/observable/interval';
import { BehaviorSubject } from 'rxjs/BehaviorSubject';
import { queue } from 'rxjs/scheduler/queue';

const source$ = new BehaviorSubject(0);

source$.observeOn(queue).subscribe();

interval(50).subscribe(source$);

Create two heap allocation snapshots about 20 seconds apart and use the comparison tools to see the allocation delta. Example:

Expected behavior:
No memory leak of previous values.

Actual behavior:
Previous values are retained in QueueActions

The text was updated successfully, but these errors were encountered:

… instance - relates to ReactiveX#2244

kwonoj · 2017-01-02T23:11:42Z

So the issue seems occurring on observeOn operator by holding observed notification's subscription. Please check #2245 for reference, local verification shows leaks are gone with given changes.

(and this can be reproduced with any other scheduler like async via observeOn)

… instance - relates to ReactiveX#2244

MikeRyanDev · 2017-01-03T18:57:49Z

@kwonoj Pulled down the latest from your branch and I am still seeing a leak of QueueActions. Also seeing a leak of instances of ObserveOnContext. All are being held by ObserveOnSubscriber._subscriptions

kwonoj · 2017-01-03T19:01:27Z

@MikeRyan52 can you share setup you've used to repro issue (maybe github repo)?

MikeRyanDev · 2017-01-03T19:22:02Z

@kwonoj I created a demo repository here that reproduces the problem with both the latest published release of rxjs and your branch: https://github.com/MikeRyan52/rxjs-memory-leak

benlesh · 2017-01-03T22:28:50Z

I see the problem right now and I'm working on a solution.

benlesh · 2017-01-04T00:19:06Z

@MikeRyan52 try the PR #2248 ... it should solve your problem.

Thank you so much for reporting this one.

MikeRyanDev · 2017-01-04T13:12:46Z

@kwonoj @Blesh Thanks for the quick responses and fix!

benlesh · 2017-01-04T17:36:15Z

@MikeRyan52 no problem. Thanks for finding this. The patch will be out soon.

The idea here is to promote a more consistent scheduling of each value across multiple browsers. It seems there were some concurrency problems when this test was run in Microsoft Edge. Hopefully this clears it up. related #2244

benlesh · 2017-01-06T00:14:30Z

@MikeRyan52 the patch is published. 5.0.3

JamesHenry · 2017-01-06T14:23:28Z

Thanks @Blesh and @MikeRyan52!

@Blesh, just FYI the latest on npm appears to still be 5.0.2

benlesh · 2017-01-06T15:33:53Z

@JamesHenry oops it looks like I published to Netflix's internal registry and not npm. Fixed.

JamesHenry · 2017-01-06T15:38:06Z

Thanks!

…event operators from leaking ChildSubscriptions. The addition of ChildSubscription to fix ReactiveX#2244 accidentally introduced a different memory leak. Most operators that add and remove inner Subscriptions store the inner Subscriber instance, not the value returned by Subscription#add. When they try to remove the inner Subscription manually, nothing is removed, because the ChildSubscription wrapper instance is the one added to the subscriptions list. Fixes ReactiveX#2355

…event operators from leaking ChildSubscriptions. (#2360) The addition of ChildSubscription to fix #2244 accidentally introduced a different memory leak. Most operators that add and remove inner Subscriptions store the inner Subscriber instance, not the value returned by Subscription#add. When they try to remove the inner Subscription manually, nothing is removed, because the ChildSubscription wrapper instance is the one added to the subscriptions list. Fixes #2355

…event operators from leaking ChildSubscriptions. (ReactiveX#2360) The addition of ChildSubscription to fix ReactiveX#2244 accidentally introduced a different memory leak. Most operators that add and remove inner Subscriptions store the inner Subscriber instance, not the value returned by Subscription#add. When they try to remove the inner Subscription manually, nothing is removed, because the ChildSubscription wrapper instance is the one added to the subscriptions list. Fixes ReactiveX#2355

* fix(timeout): Cancels scheduled timeout, if no longer needed fixes #2134 * fix(timeoutWith): Cancels scheduled timeout, if no longer needed * build(npm-scripts): update debug_mocha npm script for node 6 * fix(VirtualAction): Block rescheduled VirtualActions from executing their scheduled work. VirtualActions are immutable so they can be inspected by the TestScheduler. In order to mirror rescheduled stateful Actions, rescheduled VirtualActions shouldn't execute if they've been rescheduled before execution. * fix(timeout): Update timeout and timeoutWith to recycle their scheduled timeout actions. The timeout and timeoutWith operators should dispose their scheduled timeout actions on unsubscription. Also, given the new scheduling architecture, they can recycle their scheduled actions so just one action is allocated per subscription. * test(timeout): Add types to timeout and timeoutWith specs * Fix merge conflicts * Fix timeoutWith to work with new Subscriber leak fix. * fix(timeout-spec): fix merge conflicts * fix(Subscription): fold ChildSubscription logic into Subscriber to prevent operators from leaking ChildSubscriptions. (#2360) The addition of ChildSubscription to fix #2244 accidentally introduced a different memory leak. Most operators that add and remove inner Subscriptions store the inner Subscriber instance, not the value returned by Subscription#add. When they try to remove the inner Subscription manually, nothing is removed, because the ChildSubscription wrapper instance is the one added to the subscriptions list. Fixes #2355 * chore(publish): 5.1.1 * Ignore coverage It's 5.5mb that people installing this don't need :) * feat(AjaxObservable) : support 'PATCH' request type Add support of the 'PATCH' request type based on the already existing 'PUT' request. * fix(subscribeToResult): accept array-like as result Accept array-like as a result to subscribe, so that Observable.from and operators using subscribeToResult have identical behaviour. * chore(ajax.patch): Adds test for ajax.patch * fix(forkJoin): add type signature for single observable with selector Add type signature for using forkJoin with single observable as first parameter and selector function as second parameter, so that typings would not prevent usage which is permitted and properly handled by operator. Closes #2347 * feat(webSocket): Add binaryType to config object Add binaryType to config object, so that it is possible to set that parameter on underlying socket before any data emits happen. Closes #2353 * fix(merge): return Observable when called with single lowerCaseO Return Observable when merge is called with single lower case observable, so that merge would always return Observable instance. * fix(bindNodeCallback): emit undefined when callback has no success arguments Emit undefined insteady of empty array by resulting Observable, when callback function is called without success parameters. Closes #2254 * chore(danger): update dangerfile to validate commit message * chore(*): correctly scope disabled `max-line-length` tslint rule The max line length is set to 150 in 'tslint.json'. In specific regions, it is desirable to allow longer lines, so these regions should be wrapped in comments like the following: ```js // Max line length enforced here. /* tslint:disable:max-line-length */ // Max line length NOT enforced here. /* tslint:enable:max-line-length */ <-- CORRECT // Max line length enforced here. ``` In many cases, the re-enabling comment incorrectly included `disable` instead of `enable` (as shown below), which essentially keeps the `max-line-length` **disabled** for the rest of the file: ```js // Max line length enforced here. /* tslint:disable:max-line-length */ // Max line length NOT enforced here. /* tslint:disable:max-line-length */ <-- INCORRECT // Max line length NOT enforced here. ``` This commit fixes these comments, so the `max-line-length` rule is properly enforced in regions that don't need longer lines. * fix(bindCallback): emit undefined when callback is without arguments In resulting Observable emit undefined when callback is called without parameters, instead of emitting empty array. * fix(mergeAll): introduce variant support <T, R> for mergeMap - closes #2372 * feat(windowTime): maxWindowSize parameter in windowTime operator Adds new parameter in windowTime operator to control how much values given window can emit. Closes #1301 * docs(ObservableInput): add ObservableInput and SubscribableOrPromise descriptions Add ObservableInput and SubscribableOrPromise interface descriptions, as well as link these interfaces in type descriptions of operators, so that users always know what kind of parameters they can pass to used methods. * fix(timeoutWith): update timeoutWith to work with new Subscriber leak fix changes

lock · 2018-06-06T20:41:16Z

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.

MikeRyanDev mentioned this issue Jan 2, 2017

Memory leak for dispatcher ngrx/store#167

Closed

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 2, 2017

fix(observeOn): remove observed subscription to clean up notification…

ca4343e

… instance - relates to ReactiveX#2244

kwonoj mentioned this issue Jan 2, 2017

fix(observeOn): remove observed subscription to clean up notification… #2245

Closed

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 2, 2017

fix(observeOn): remove observed subscription to clean up notification…

3e6ce97

… instance - relates to ReactiveX#2244

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 2, 2017

fix(observeOn): remove observed subscription to clean up notification…

686082d

… instance - relates to ReactiveX#2244

kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 2, 2017

fix(observeOn): remove observed subscription to clean up notification…

ee25160

… instance - relates to ReactiveX#2244

benlesh self-assigned this Jan 3, 2017

benlesh mentioned this issue Jan 4, 2017

Observe on leak #2248

Merged

benlesh closed this as completed in #2248 Jan 4, 2017

benlesh mentioned this issue Jan 5, 2017

refactor(observeOn-spec): use a scheduler instead of setInterval #2252

Merged

trxcllnt mentioned this issue Feb 12, 2017

ChildSubscriptions unsubscribed but not removed. #2355

Closed

trxcllnt mentioned this issue Feb 12, 2017

fix(Subscription): fold ChildSubscription logic into Subscriber to prevent operators from leaking ChildSubscriptions. #2360

Merged

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory Leak with Queue Scheduler and Subject #2244

Memory Leak with Queue Scheduler and Subject #2244

MikeRyanDev commented Jan 2, 2017 •

edited

Loading

kwonoj commented Jan 2, 2017 •

edited

Loading

MikeRyanDev commented Jan 3, 2017

kwonoj commented Jan 3, 2017

MikeRyanDev commented Jan 3, 2017

benlesh commented Jan 3, 2017

benlesh commented Jan 4, 2017

MikeRyanDev commented Jan 4, 2017

benlesh commented Jan 4, 2017

benlesh commented Jan 6, 2017

JamesHenry commented Jan 6, 2017

benlesh commented Jan 6, 2017

JamesHenry commented Jan 6, 2017

lock bot commented Jun 6, 2018

Memory Leak with Queue Scheduler and Subject #2244

Memory Leak with Queue Scheduler and Subject #2244

Comments

MikeRyanDev commented Jan 2, 2017 • edited Loading

kwonoj commented Jan 2, 2017 • edited Loading

MikeRyanDev commented Jan 3, 2017

kwonoj commented Jan 3, 2017

MikeRyanDev commented Jan 3, 2017

benlesh commented Jan 3, 2017

benlesh commented Jan 4, 2017

MikeRyanDev commented Jan 4, 2017

benlesh commented Jan 4, 2017

benlesh commented Jan 6, 2017

JamesHenry commented Jan 6, 2017

benlesh commented Jan 6, 2017

JamesHenry commented Jan 6, 2017

lock bot commented Jun 6, 2018

MikeRyanDev commented Jan 2, 2017 •

edited

Loading

kwonoj commented Jan 2, 2017 •

edited

Loading