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

trxcllnt · 2017-02-12T07:00:32Z

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, and supersedes the fix in #2358. Thanks again to @johncvrs for reporting this.

coveralls · 2017-02-12T07:05:13Z

Coverage increased (+0.02%) to 97.705% when pulling c02bc8f on trxcllnt:fix-child-subscriptions into 31dfc73 on ReactiveX:master.

jayphelps · 2017-02-13T03:40:20Z

src/Subscription.ts

+    // to remove themselves from this subscription will noop
+    this._subscriptions = null;
+
+    let index = -1, len = _parents && _parents.length || 0;


IMO we should prefer multiline declarations for clarity--that's just my opinion tho.

jayphelps · 2017-02-13T03:42:19Z

src/Subscription.ts

+      _parent.remove(this);
+      // if this._parents is null or index >= len,
+      // then _parent is set to false, and the loop exits
+      _parent = ++index < len && _parents[index];


I think this is harder to grok than neccesary--shouldn't need a comment that describes what it does.

jayphelps · 2017-02-13T03:44:46Z

src/Subscription.ts

+    let { _parent, _parents } = this;
+    if (!_parent) {
+      this._parent = parent;
+    } else if (_parents || (_parents = this._parents = [])) {


Can we move this assignment out of the conditional and into the block instead?

Something like

} else { if (!_parents) { _parents = this._parents = []; } _parents.push(this); }

I get that these checks are annoying, perhaps there's a better place to set defaults.

benlesh · 2017-02-13T03:45:05Z

Pending @jayphelps' requests, it LGTM

…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

jayphelps

LGTM now, based on conversations in Slack as well.

I'd like to bikeshed _parent vs _parents stuff some more, but separate to this PR.

coveralls · 2017-02-13T04:21:51Z

Coverage increased (+0.02%) to 97.705% when pulling c02bc8f on trxcllnt:fix-child-subscriptions into 31dfc73 on ReactiveX:master.

benlesh · 2017-02-13T05:19:50Z

The coveralls change looks like unfortunate math. This fix is too critical to block over a 0.01% coverage change. I'm going to go ahead and merge.

…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-06T14:40:45Z

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.

jayphelps reviewed Feb 13, 2017

View reviewed changes

benlesh approved these changes Feb 13, 2017

View reviewed changes

jayphelps reviewed Feb 13, 2017

View reviewed changes

trxcllnt force-pushed the fix-child-subscriptions branch from c02bc8f to 9567ca0 Compare February 13, 2017 04:17

jayphelps approved these changes Feb 13, 2017

View reviewed changes

benlesh merged commit 22e4c17 into ReactiveX:master Feb 13, 2017

trxcllnt mentioned this pull request Feb 13, 2017

fix(switch): stop leaking ChildSubscriptions #2358

Closed

johncvrs added a commit to johncvrs/rxjs that referenced this pull request Feb 13, 2017

docs(UML): Remove ChildSubscriptions per change in PR ReactiveX#2360.

846f5ff

janmarthedal mentioned this pull request Feb 23, 2017

Memory leak when using concatMap with asynchronous function #2342

Closed

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

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

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

trxcllnt commented Feb 12, 2017

coveralls commented Feb 12, 2017

jayphelps Feb 13, 2017

jayphelps Feb 13, 2017

jayphelps Feb 13, 2017

jayphelps Feb 13, 2017

benlesh commented Feb 13, 2017

jayphelps left a comment

coveralls commented Feb 13, 2017 •

edited

Loading

benlesh commented Feb 13, 2017

lock bot commented Jun 6, 2018

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

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

Conversation

trxcllnt commented Feb 12, 2017

coveralls commented Feb 12, 2017

jayphelps Feb 13, 2017

Choose a reason for hiding this comment

jayphelps Feb 13, 2017

Choose a reason for hiding this comment

jayphelps Feb 13, 2017

Choose a reason for hiding this comment

jayphelps Feb 13, 2017

Choose a reason for hiding this comment

benlesh commented Feb 13, 2017

jayphelps left a comment

Choose a reason for hiding this comment

coveralls commented Feb 13, 2017 • edited Loading

benlesh commented Feb 13, 2017

lock bot commented Jun 6, 2018

coveralls commented Feb 13, 2017 •

edited

Loading