fix(delay): fix memory leak #3605

ubnt-michals · 2018-04-26T09:22:22Z

Description:
Hopefully fix leaking memory for delay operator

EDIT:
I had to bend the VirtualTimeScheduler to not shift the last action in order to properly test this. This could be potential breaking change if left as it is. I am not sure how to test the case without the change though. Any help is appreciated.

Related issue (if exists):
#3604

coveralls · 2018-04-26T09:31:39Z

Coverage increased (+0.2%) to 96.727% when pulling 8afb492 on ubnt-michals:delay_leak_fix into 8c5d680 on ReactiveX:master.

benlesh

This PR probably isn't the right solution.

The issue it's referencing isn't quite refined enough to be sure it's an issue with delay.
Making changes to the VirtualTimeScheduler in this case is a big no-no, as that can effect almost all tests in our test suite. :)
The fix that is here may have issues when simultaneous delays are scheduled.

benlesh · 2018-04-26T16:23:14Z

src/scheduler/VirtualTimeScheduler.ts

@@ -24,7 +24,15 @@ export class VirtualTimeScheduler extends AsyncScheduler {
    const {actions, maxFrames} = this;
    let error: any, action: AsyncAction<any>;

-    while ((action = actions.shift()) && (this.frame = action.delay) <= maxFrames) {
+    while (actions.length > 0) {


To fix delay, we shouldn't have done anything to the VirtualTimeScheduler, changes to the virtual scheduler will affect all marble tests.

This change has nothing to do with the fix. It's just I wanted to test that the last action is correctly unsubscribed and when calling scheduler.flush() it removed the action that wasn't executed

actions.length === 1 maxFrames === 60 action.delay === 80 (action = actions.shift()) && (this.frame = action.delay) <= maxFrames true && false // no execution actions.length === 0 // same as if action would execute

benlesh · 2018-04-26T16:24:37Z

src/operators/delay.ts

@@ -79,7 +79,7 @@ interface DelayState<T> {
 */
 class DelaySubscriber<T> extends Subscriber<T> {
  private queue: Array<DelayMessage<T>> = [];
-  private active: boolean = false;
+  private nextAction: Subscription = null;


i'm suspicious this will have issues in situations where more than one delayed action is dispatched synchronously. For example of(1, 2, 3).pipe(delay(10)) or the like. You'd have to keep more than one subscription for each scheduled action.

The original code would only schedule one action. this.active would be set to true and additional messages would be buffered until dispatch. On dispatch the queue would be processed... etc. I hope I am interpreting it correctly...

I think this code does the same except it doesn't register unsubscription logic that is the cause of the leak.

cartant · 2018-04-26T23:29:55Z

@benlesh I'll review this later today. If sinon is used, it should be testable without modifying the scheduler.

cartant

Yeah, it looks like you've found a bug. Thanks for the work you've put into this.

I can see how your fix works, but I think there is a simpler alternative.

The leak appears to arise from the action being added the the DelaySubscriber's subscriptions without ever being removed. And you have fixed this by storing the subscription (the action) in a property - which means you also have to implement unsubscribe to take this into account.

IMO, the fix should be a one-liner: just add source.remove(this); after source.active = false; in DelaySubscriber#dispatch.

And you should not need to modify the VirtualTimeScheduler to test this. A test something like this should do it:

let subscriber: any = null;
let subscribeSpy: any = null;
const counts: number[] = [];

const duration = time('-|');
const result = timer(duration).pipe(
    repeatWhen(notifications => {
        const delayed = notifications.pipe(delay(duration));
        subscribeSpy = sinon.spy(delayed["source"], "subscribe");
        return delayed;
    }),
    skip(1),
    take(5),
    tap(() => {
        const [[subscriber]] = subscribeSpy.args;
        counts.push(subscriber._subscriptions.length);
    })
);

expectObservable(result).toBe(/* work out the expected diagram */);
expect(counts).to.deep.equal([1, 1, 1, 1, 1]);

And, ideally, the (failing) test should be added in a separate commit that precedes the fix.

cartant · 2018-04-27T07:17:53Z

src/operators/delay.ts

@@ -96,7 +96,8 @@ class DelaySubscriber<T> extends Subscriber<T> {
      const delay = Math.max(0, queue[0].time - scheduler.now());
      this.schedule(state, delay);
    } else {
-      source.active = false;


I think the fix is a one-liner and source.remove(this); should be added here.

Good point, I've totally missed that. Will fix the PR. Thanks.

benlesh · 2018-04-27T18:32:12Z

Thank you, @cartant, for taking the time to review this in more detail.

cartant

LGTM. However, I've suggested a couple of minor changes.

I should have suggested this.unsubscribe() be used in the first place - apologies for the hassle - but it wasn't until I was trying to come up with an accurate test description for the test that I realised that this.unsubscribe() was more idiomatic and was the way to go. (The behaviour will be the same, as the implementation of Subscription#unsubscribe will remove the subscription from its parent.)

cartant · 2018-04-29T23:07:06Z

src/internal/operators/delay.ts

@@ -92,6 +92,7 @@ class DelaySubscriber<T> extends Subscriber<T> {
      const delay = Math.max(0, queue[0].time - scheduler.now());
      this.schedule(state, delay);
    } else {
+      source.remove(this);


I've changed my mind with this. I think this.unsubscribe() instead of source.remove(this) would be better. The effected behaviour will be identical, but the former is more idiomatic.

cartant · 2018-04-29T23:10:19Z

spec/operators/delay-spec.ts

@@ -143,4 +146,33 @@ describe('Observable.prototype.delay', () => {

    expectObservable(result).toBe(expected);
  });
+
+  it('should unsubscribe scheduled action when result is unsubscribed explicitly', () => {


One minor nit: the description doesn't actually describe the problem. The leak happens because the actions are not removed/unsubscribed from the subscriber's internal subscriptions after the action is executed. I think a more accurate description would be something like should unsubscribe scheduled actions after execution.

The actions will be unsubscribed when the subscriber unsubscribes; it's just that they will accumulate up until that happens.

ubnt-michals · 2018-05-03T06:35:44Z

@cartant All should be fixed now. I've learned a lot finding and fixing this bug. Thanks for your time.

cartant

Thanks. LGTM.

benlesh · 2018-05-03T16:27:00Z

Thank you, @ubnt-michals! (and thanks for the review @cartant )

* test(delay): failing test * fix(delay): fix memory leak

lock · 2018-06-05T19:35:37Z

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.

benlesh requested changes Apr 26, 2018

View reviewed changes

benlesh requested a review from cartant April 26, 2018 16:30

cartant requested changes Apr 27, 2018

View reviewed changes

test(delay): failing test

60e7521

ubnt-michals force-pushed the delay_leak_fix branch from 51995e0 to 0ced8f3 Compare April 29, 2018 11:30

ubnt-michals changed the base branch from stable to master April 29, 2018 11:30

ubnt-michals force-pushed the delay_leak_fix branch 3 times, most recently from f6810fa to 06db858 Compare April 29, 2018 11:42

cartant reviewed Apr 29, 2018

View reviewed changes

fix(delay): fix memory leak

8afb492

ubnt-michals force-pushed the delay_leak_fix branch from 06db858 to 8afb492 Compare May 3, 2018 06:33

cartant approved these changes May 3, 2018

View reviewed changes

benlesh approved these changes May 3, 2018

View reviewed changes

benlesh merged commit 96f05b0 into ReactiveX:master May 3, 2018

ubnt-michals mentioned this pull request May 4, 2018

Delay operator will leak memory #3604

Closed

cartant pushed a commit to cartant/rxjs that referenced this pull request May 7, 2018

fix(delay): fix memory leak (ReactiveX#3605)

6a2a7f8

* test(delay): failing test * fix(delay): fix memory leak

cartant mentioned this pull request May 8, 2018

fix(delay): fix memory leak (in stable) #3667

Merged

cartant pushed a commit to cartant/rxjs that referenced this pull request May 8, 2018

fix(delay): fix memory leak (ReactiveX#3605)

9d1ba14

* test(delay): failing test * fix(delay): fix memory leak

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(delay): fix memory leak #3605

fix(delay): fix memory leak #3605

ubnt-michals commented Apr 26, 2018 •

edited

Loading

coveralls commented Apr 26, 2018 •

edited

Loading

benlesh left a comment

benlesh Apr 26, 2018

ubnt-michals Apr 26, 2018

benlesh Apr 26, 2018

ubnt-michals Apr 26, 2018

cartant commented Apr 26, 2018

cartant left a comment

cartant Apr 27, 2018

ubnt-michals Apr 27, 2018

benlesh commented Apr 27, 2018

cartant left a comment

cartant Apr 29, 2018

cartant Apr 29, 2018 •

edited

Loading

ubnt-michals commented May 3, 2018

cartant left a comment

benlesh commented May 3, 2018

lock bot commented Jun 5, 2018

fix(delay): fix memory leak #3605

fix(delay): fix memory leak #3605

Conversation

ubnt-michals commented Apr 26, 2018 • edited Loading

coveralls commented Apr 26, 2018 • edited Loading

benlesh left a comment

Choose a reason for hiding this comment

benlesh Apr 26, 2018

Choose a reason for hiding this comment

ubnt-michals Apr 26, 2018

Choose a reason for hiding this comment

benlesh Apr 26, 2018

Choose a reason for hiding this comment

ubnt-michals Apr 26, 2018

Choose a reason for hiding this comment

cartant commented Apr 26, 2018

cartant left a comment

Choose a reason for hiding this comment

cartant Apr 27, 2018

Choose a reason for hiding this comment

ubnt-michals Apr 27, 2018

Choose a reason for hiding this comment

benlesh commented Apr 27, 2018

cartant left a comment

Choose a reason for hiding this comment

cartant Apr 29, 2018

Choose a reason for hiding this comment

cartant Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

ubnt-michals commented May 3, 2018

cartant left a comment

Choose a reason for hiding this comment

benlesh commented May 3, 2018

lock bot commented Jun 5, 2018

ubnt-michals commented Apr 26, 2018 •

edited

Loading

coveralls commented Apr 26, 2018 •

edited

Loading

cartant Apr 29, 2018 •

edited

Loading