39

I have a webapp written in AngularJS which basically polls an API to two endpoints. So, every minute it polls to see if there is anything new.

I discovered that it has a small memory leak and I've done my best to find it but I'm not able to do it. In the process I've managed to reduce the memory usage of my app, which is great.

Without doing anything else, every poll you can see a spike in the memory usage (that's normal) and then it should drop, but it's always increasing. I've changed the cleaning of the arrays from [] to array.length = 0 and I think I'm sure that references don't persist so it shouldn't be retaining any of this.

I've also tried this: https://github.com/angular/angular.js/issues/1522

But without any luck...

So, this is a comparison between two heaps:

Memory heap

Most of the leak seems to come from (array) which, if I open, are the arrays returned by the parsing of the API call but I'm sure they're not being stored:

This is basically the structure:

poll: function(service) {
  var self = this;
  log('Polling for %s', service);

  this[service].get().then(function(response) {
    if (!response) {
      return;
    }

    var interval = response.headers ? (parseInt(response.headers('X-Poll-Interval'), 10) || 60) : 60;

    services[service].timeout = setTimeout(function(){
      $rootScope.$apply(function(){
        self.poll(service);
      });
    }, interval * 1000);

    services[service].lastRead = new Date();
    $rootScope.$broadcast('api.'+service, response.data);
  });
}

Basically, let's say I have a sellings service so, that would be the value of the service variable.

Then, in the main view:

$scope.$on('api.sellings', function(event, data) {
  $scope.sellings.length = 0;
  $scope.sellings = data;
});

And the view does have an ngRepeat which renders this as needed. I spent a lot of time trying to figure this out by myself and I couldn't. I know this is a hard issue but, do anyone have any idea on how to track this down?

Edit 1 - Adding Promise showcase:

This is makeRequest which is the function used by the two services:

return $http(options).then(function(response) {
    if (response.data.message) {
      log('api.error', response.data);
    }

    if (response.data.message == 'Server Error') {    
      return $q.reject();
    }

    if (response.data.message == 'Bad credentials' || response.data.message == 'Maximum number of login attempts exceeded') {
      $rootScope.$broadcast('api.unauthorized');
      return $q.reject();
    }

    return response;
    }, function(response) {
    if (response.status == 401 || response.status == 403) {
      $rootScope.$broadcast('api.unauthorized');
    }
});

If I comment out the $scope.$on('api.sellings') part, the leakage still exists but drops to 1%.

PS: I'm using latest Angular version to date

Edit 2 - Opening (array) tree in an image

enter image description here

It's everything like that so it's quite useless imho :(

Also, here are 4 heap reports so you can play yourself:

https://www.dropbox.com/s/ys3fxyewgdanw5c/Heap.zip

Edit 3 - In response to @zeroflagL

Editing the directive, didn't have any impact on the leak although the closure part seems to be better since it's not showing jQuery cache things?

No more leakage?

The directive now looks like this

var destroy = function(){
  if (cls){
    stopObserving();
    cls.destroy();
    cls = null;
  }
};

el.on('$destroy', destroy);
scope.$on('$destroy', destroy);

To me, it seems that what's happening is on the (array) part. There is also 3 new heaps in between pollings.

Antonio Laguna
  • 8,973
  • 7
  • 36
  • 72
  • My bet is on Promise hanging onto your then() callback. Did you already trace down into Promise? – Ezekiel Victor Dec 18 '13 at 10:02
  • Thanks @EzekielVictor - I've updated my answer. That's the same thing I thought but honestly, after that I don't think it's there! – Antonio Laguna Dec 18 '13 at 10:43
  • Can you show the retaining tree of one of the arrays that should have been GC'd ? – Pieter Herroelen Dec 20 '13 at 10:46
  • Done @PieterHerroelen I've also added heap reports – Antonio Laguna Dec 20 '13 at 11:41
  • Could it be that it's because you call 'poll()' recursively? What happens if you set `response` to `null` after the `$broadcast()`? Could this recursion not cause a stack overflow? – Pieter Herroelen Dec 20 '13 at 12:52
  • Why would recursion create an stack overflow? There is a space in between of one minute. Are you suggesting an infinite closure? Anyway I've tried setting response to null after broadcast and results are still similar... – Antonio Laguna Dec 20 '13 at 13:33
  • 3
    @AntonioLaguna Anyway you can reproduce this in a plunker/fiddle ? – Beyers Dec 23 '13 at 10:16
  • +1 @Beyers this is impossible to solve with what you gave us... what owns polls (service? ). you also dont handle the error case? – Nix Dec 24 '13 at 01:44
  • "So, this is a comparison between two heaps" - The image shows the content of one heap only ^^ The snapshots you've provided show no change. I guess the time between them was too short (< 1 min). "Most of the leak seems to come from (array) which, if I open ..." If there is a leak then you can see its cause there. – a better oliver Dec 25 '13 at 16:37
  • How can I possibly put all the app in a plunker? I can't. I'm asking on how to interprete the information provided. Also @zeroflagL it's two heaps as you won't see retained size in other situation. The time between the heaps is about 1 minute and 10 seconds since I waited for every poll to happen in order to capture the heap since there is where I suspected the leak to be. – Antonio Laguna Dec 25 '13 at 21:03
  • @Antonio the idea is not to put your entire app inside a plunker, but to create a small sample that illustrates the issue. If you are unwilling to put in that effort then dont expect much help around here. – Beyers Dec 25 '13 at 22:26
  • Retained size is the size of the object itself plus the size of the objects referenced by it. A comparison view shows the number of objects that have been created and deleted between two snapshots. Your 4 snapshots have the timestamps 11:37:37 / 47 / 53 / 58. That is 21 seconds between the first and the last one. Add a snapshot from both before and after a service call and we will analyze it and explain every detail in an answer. So far nothing indicates a memory leak. Increasing memory usage is normal in managed environments. – a better oliver Dec 25 '13 at 22:57
  • @zeroflagL that must be the date I saved them and not the date in which were generated since I'm totally sure I waited 1 minute. However, I'll do as you request, that makes sense! – Antonio Laguna Dec 26 '13 at 13:59
  • You are right about the timestamps. And I# – a better oliver Dec 26 '13 at 14:26
  • I've discovered something: Chrome doesn't seem to be able to compare saved snapshots. So unfortunately you can only compare them yourself. – a better oliver Dec 26 '13 at 14:35

2 Answers2

28

And the answer is cache.

Heap Snapshot analysis

I don't know what it is, but this thing grows. It seems to be related to jQuery. Maybe it's the jQuery element cache. Do you by any chance apply a jQuery plugin on one or more elements after every service call?

Update

The problem is that HTML elements are added, processed with jQuery (e.g. via the popbox plugin), but either never removed at all or not removed with jQuery. To process in this case means stuff like adding event handlers. The entries in the cache object (whatever it is for) do only get removed if jQuery knows that the elements have been removed. That is the elements have to be removed with jQuery.

Update 2

It's not quite clear why these entries in the cache haven't been removed, as angular is supposed to use jQuery, when it's included. But they have been added through the plugin mentioned in the comments and contained event handlers and data. AFAIK Antonio has changed the plugin code to unbind the event handlers and remove the data in the plugin's destroy() method. That eventually removed the memory leak.

a better oliver
  • 26,330
  • 2
  • 58
  • 66
  • Yes I do, there is a directive which uses a popup. However, the directive does have this function: `scope.$on('$destroy', function(){ stopObserving(); plugin.destroy(); });` you can see it here: https://github.com/firstandthird/angular-popbox/blob/master/dist/angular-popbox.js – Antonio Laguna Dec 26 '13 at 15:13
  • 1
    @AntonioLaguna `var $el = $(el);` - `el` already should be a jQuery object. Maybe that's the cause. Try to use `el` directly. – a better oliver Dec 26 '13 at 15:26
  • It seems it's not. If I remove that, the plugin will complain saying that [Object object] doesn't have method `popbox` – Antonio Laguna Dec 26 '13 at 17:27
  • @AntonioLaguna Yeah, it's only a jqLite object, which is really sad IMHO. I think I found the root cause. I updated my answer. – a better oliver Dec 26 '13 at 18:39
  • If you're right, how can I solve that? I'm getting some errors on the `$destroy` since the element is already nullified (I suspect that happens on the `ngRepeat`) All of that is handled by ngRepeat which *should* be clean? – Antonio Laguna Dec 26 '13 at 19:00
  • You have to use `$el.bind('$destroy'...) ` for that. It's a DOM-event triggered before the element is destroyed. You will have to clean up manually then, i.e. undo everything that has been done with jQuery, like unbind event handlers, remove data etc. Maybe it's enough to remove the child elements with jQuery, like `$el.children().remove()`. Remember, it's a **jQuery internal cache**! It doesn't help if things are cleaned up outside of jQuery, the cache is the problem. – a better oliver Dec 26 '13 at 19:26
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/43947/discussion-between-antonio-laguna-and-zeroflagl) – Antonio Laguna Dec 26 '13 at 20:23
  • @AntonioLaguna zeroflagL is correct, you shouldn't have to wrap the element passed into the link function with jQuery as it will already be a jQuery object. The documentation is clear on this http://docs.angularjs.org/api/angular.element also see point 5 of this popular answer http://stackoverflow.com/a/15012542/202913 The only thing you need to do is load jQuery *before* angularjs. – Beyers Dec 26 '13 at 20:24
  • @Beyers I'm sure jQuery is loading *before* AngularJS and if I try with that suggestion the plugin just won't work... However it seems weird. Also, that isn't likely to create a leak since if you take a look into the jQuery function, its creating a new instance. The previous will loose reference and hence will be GCed. I've checked quickly with a Plunk and there is no leak on doing that again and again. http://plnkr.co/edit/71b1MRcMY0k4fhAMITqC?p=preview – Antonio Laguna Dec 26 '13 at 20:41
3

The standard browser way to fix memory leaks is to refresh the page. And JavaScript garbage collection is kind of lazy, likely banking on this. And since Angular is typically a SPA, the browser never gets a chance to refresh.

But we have 1 thing to our advantage: Javascript is primarily a top-down hierarchial language. Instead of searching for memory leaks from the bottom up, we may be able to clear them from the top down.

Therefore I came up with this solution, which works, but may or may not be 100% effective depending on your app.

The Home Page

The typical Angular app home page consists of some Controller and ng-view. Like this:

<div ng-controller="MainController as vm"> <div id="main-content-app" ng-view></div> </div>

The Controller

Then to "refresh" the app in the controller, which would be MainController from the code above, we redundantly call jQuery's .empty() and Angular's .empty() just to make sure that any cross-library references are cleared.

function refreshApp() {
var host = document.getElementById('main-content-app');
if(host) {
    var mainDiv = $("#main-content-app");
    mainDiv.empty();
    angular.element(host).empty();
}
}

and to call the above before routing begins, simulating a page refresh:

$rootScope.$on('$routeChangeStart',
function (event, next, current) {
    refreshApp();
}
);

Result

This is kind of a hacky method for "refreshing the browser type behavior", clearing the DOM and hopefully any leaks. Hope it helps.

jmbmage
  • 2,487
  • 3
  • 27
  • 44
  • 3
    Although I appreciate the effort put into this answer and into the code you've written, I think this is (as you state) kind of hacky. You're correct stating that the way the browser has to eliminate the leaks is to refresh the page. However, as developers, we have to create code that **doesn't leak**. With SPA nowadays more than ever since the page may not be refreshed ever again and the app should work without causing havoc on the browser. – Antonio Laguna Aug 28 '14 at 06:20
  • 1
    I agree completely,but I can almost guarantee you that as apps get bigger and more demanding, this will become an issue as caches build, etc. **And the issue is not always in the code, but in the browser's Garbage Collection!** The last 2 weeks the company I've been working for has been testing a massive Angular app that starts at about 300mb of data in the browser (as a test). IE 11's Garbage Collection is currently handling it the best. – jmbmage Aug 29 '14 at 12:38