Rxjs: Observable memory leak with long polling

[Guardiannw]

I am trying to do a long polling example using angular2 and observables. I had basically this snippet in order to perform the long polling.

(<KitchenSinkOperators<any>>Observable.interval(50))
            .exhaustMap(() => this.getResourceFromBackend('state'))
            .map((resource) => resource.value)
            .distinctUntilChanged()
            .startWith(0);

This code created a MASSIVE memory leak when run in my various browsers, it kept climbing and climbing.

Yet, when I change that code out to do the same thing with Observable.create, everything works as expected. This is the converted version of the above code:

return Observable.create((subscriber: Subscriber<number>) => {
            let register = 0;
            let subscription: Subscription = null;
            // Set an initial value of 0
            subscriber.next(register);

            let interval = setInterval(() => {
                if(subscription === null) {
                    subscription = this.getResourceFromBackend('state')
                        .subscribe(({value}) => {
                            if (<number>value !== register)
                                subscriber.next(register = <number>value);
                        }, (err: any) => subscriber.error(err), () => { subscription.unsubscribe(); subscription = null; });
                }
            }, 50);

            return () => {
                clearInterval(interval);
                if(subscription !== null)
                    subscription.unsubscribe();
            }
        });

Am I misunderstanding something about how Observables and their operators work? Or is there truly a bug here that needs to be fixed? For now it seems I have to stick with my custom implementation, although I would rather not if we can fix the initial code.

Thank you very much,

[trxcllnt]

@Guardiannw do you have a test case I can run in isolation to reproduce the memory leak? I ran a test on Observable.interval and haven't run into any leaks, but it could be somewhere else.

[Guardiannw]

@trxcllnt Thanks for getting back to me, I will try to get one on plunkr here in the next few minutes.

[Guardiannw]

@trxcllnt Here is the plnkr, but I haven't got to thoroughly test it to make sure it is having the same results.

[Guardiannw]

@trxcllnt @blesh I was able to confirm that the example I gave was giving a memory leak on both Chrome and iOS.

In chrome it kept climbing from 29mb up to 91mb used in a period of about 10 minutes or so.

[Guardiannw]

But....It looks like even this one seems to have a memory leak

[trxcllnt]

@Guardiannw I should have asked before, but do you have a test case that doesn't use Angular? I've tried both your examples with no luck, and want to make sure it's our fault instead of Angular.

[Guardiannw]

@trxcllnt I think you are right in your assertion. While I cannot understand why the above 2 different code samples, which are essentially the same thing functionally, behave in such different ways from an efficiency standpoint. I cannot seem to accurately diagnose a memory leak when I test this apart from angular. I will try to re post this on their repo. Question though: do you know why the two code samples above might be giving such different results?

[Guardiannw]

@trxcllnt I did find what I believe to be a problem. In RxJS/src/operator/exhaustMap.ts there is no remove method being called to remove the old unsubscribed subscribers from the _subscriptions array. Won't this junk up the exhaustMap with a great deal of old subscribers that no longer serve any purpose?

Thanks,

[kwonoj]

When does inner observable returned by getResourceFromBackend completes?

[Guardiannw]

@kwonoj I believe it completes immediately after it makes an ajax request to get the resource.

[trxcllnt]

@guardiannw yes it would certainly seem that exhaustMap has omitted subscription removal, and would leak for long-lived subscriptions. They will go away if you dispose the outer subscription, but only then. @ojkwon @blesh we need to fix this asap (while perhaps adding support for a concurrent parameter?)

[kwonoj]

I'll prep PR for this, since this can be short PR and doesn't need to be aggregated with additional behavior changes.

[Guardiannw]

@trxcllnt @kwonoj Great! Thanks for the PR! I tested the changes on my box and I am still having a leak though. Albeit smaller than before. I will keep trying to look around and see if I can find anything. If there is anything that you guys could find I would appreciate continued help.

Thanks,

[kwonoj]

Seems that wasn't only culprit, I'll also try on my end as well if I can see anything suspicious.

[Guardiannw]

I have been able to identify that the other leak is coming from Observable.interval. Simply using Observable.interval alone is causing a leak; however, even when I stub it out for a functional replica,

return Observable.create((subscriber: Subscriber<number>) => {
            let register = 0;
            let interval = setInterval(() => {
                                subscriber.next(register++);
            }, 50);
            return () => {
                clearInterval(interval);
            }
        })

within my larger code in my first post above, it still shows that it it is the error. The code above has an error anytime I use Observable.interval, but there is no error when I replace it with this code. I have not yet determined what is causing Observable.interval to have a leak in this use case. But I will continue to look into it.

Thanks,

[Guardiannw]

@trxcllnt @kwonoj I was able to test this apart from angular through this plunk. In order to see what I see, run this in chrome and inspect in timeline, hit the garbage collection about once every 5 seconds and see how the minimum keeps rising.

Thanks,

[Guardiannw]

NVM...I was testing the wrong plunk. It doesn't look like that one causes a problem outside of angular.

[trxcllnt]

@Guardiannw one difference between your custom Observable and Observable.interval is that you're using setInterval, but the schedulers are repeatedly calling setTimeout (for now).

Updating the Schedulers to use setInterval has been in the back of my mind, I just haven't gotten around to it. I'm not sure whether that's in play here, but it's a possibility.

[mattpodwysocki]

@trxcllnt yes, you should do that as it's much more efficient. We have in RxJS v4 schedulePeriodic which is to be used for that purpose, also when timer when the offset and interval time match.

[Guardiannw]

@trxcllnt I saw that, and I tried to track the flow of the application throughout the different files involved in the IntervalObservable, but I think that Angular might be getting hung up on all the different loops that Observable.interval is going through. If it could be designed so that it doesn't have to go through so many loops, then I think that would be great. But this is still something the Angular team will want to pick up, as it is working as expected outside of Angular.

@mattpodwysocki I am up for solving this problem any way that works!

[trxcllnt]

@mattpodwysocki done and without splitting the public API in #1395

[benlesh]

Back to just the issue. It looks like the root cause here is a lack of clean up of the scheduled actions in a number of operators. We can probably fix that in a way that's orthogonal to @trxcllnt's proposed change.

[Guardiannw]

Awesome!

[benlesh]

@Guardiannw I spoke with @mhevery and the issue that was causing the memory leak is fixed in the latest version of zones, which they're now integrating into Angular 2. At its root, the issue was that zones.js was building a virtual call stack for nested calls to setTimeout.

I've asked @mhevery to comment here to provide more detail, but in light of all of that, I'm going to close this issue for now. Once zones is updated in NG2, if you try this again and you're still seeing the issue, don't hesitate to reopen.

This was a good issue, because it brought to light some other things like a perf win with using setInterval, etc. as well as getting some strong discussion around our scheduling API. Thanks again!

[Guardiannw]

Thanks @blesh for getting to the root of this, I really appreciate it. And thanks @trxcllnt for all your help in coming up with a solution and picking this up.

[benlesh]

@trxcllnt did all of the work. I'm just the messenger and the guy that checked with the Angular team.

[mhevery]

@blesh got the full story. I think for perf reasons you may think about the new Zone.current.scheduleMicrotask API in new Zone.js which will be the most efficient way to get a clean stack trace on any environment which does not support it natively. (And you can have fake async in tests.)

[aalbericio]

Is it possible that we're hitting this bug again?

I mean, we're using Angular 4.3.1, RxJS 5.4.1 and Zone 0.8.11

This simple subscription is creating a leak:

this._timeSubscription = Observable.interval(1000).subscribe(x => this.getCurrentTime());

What's more interesting is that running the subscription outside the angular zone (runOutsideAngular) plus manually detectingChanges does the trick!

Can someone confirm this?

Thanks!

[tao-cumplido]

Thanks @aalbericio for the hint. Running the subscription outside the angular zone was the solution for me.
I hit this problem while subscribing to multiple source.skipWhile(...).takeWhile(...) where source was created with Observable.interval(100). About a minute after running this the chormium instance inside Electron became completely unresponsive. I tried to reduce the number of subscriptions to see what happens but it didn't seem to change anything.

I am using

• Angular 4.4.0-RC.0
• RxJS 5.4.3
• Zone.js 0.8.17

[meriturva]

Same here @aalbericio . Running Observable.interval outside angular zone solve memory leak. I guess issue has to be reopen and fixed quite urgent. Thanks.

[meriturva]

new zonejs 0.8.18 version out . @aalbericio @tao-cumplido did you check it? changelog just shows a fix related to setTimeout but no ref about memory leak

[lock[bot]]

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.