6

I am making an AJAX request for XML. I am doing this every second. I notice that my memory usage grows into the hundreds of megabytes. As you might imagine, the customer is not happy with this. After doing some reading in various places, I suspect that function closures are causing my headache. I am looking for any verification that can be had as well as any help for how to fix it.

function PageManager () {
    var self = this;

    self.timeoutHandler = function () {
        $.ajax ({
            url: 'URLtoXML',
            type: 'post',
            cache: false,
            context: self,
            success: function (data) {
                var slf = this;
                var xmlDoc = $($.parseXML (data));
                xmlDoc.find ("tag_label").each (function () {
                    self.val = parseInt ($.trim ($(this).text ()));
                }
                setTimeout (slf.timeoutHandler, 750);
            }
        });
    }
}

var pm = new PageManager ();
pm.timeoutHandler ();

EDIT I have incorporated some people's ideas and a little of the success handler internals. I see a smaller growth rate, but not by much.

Nik
  • 7,113
  • 13
  • 51
  • 80
  • @edit, have you tried my solution? It shouldn't cause an infinitely growing stack. I believe that any solution with `setTimeout` will, no matter how small your scope is. A smaller scope just means it grows slower. – Halcyon Dec 01 '11 at 21:27
  • It looks like your suggestion has slowed the growth further. It's almost acceptable. I still don't see why I would have any growth. – Nik Dec 01 '11 at 21:36
  • 1
    People have been complaining about memoryleaks in jquery's XHR handling parts for years (at least 5, *afaik*), and in each new release they say "we fixed the memory leak" .. ehh ... just do the XHR manually. – tereško Dec 01 '11 at 21:36
  • All browsers or specific to one? – Incognito Dec 01 '11 at 21:37
  • 1
    Does not seem to discriminate. – Nik Dec 01 '11 at 21:39
  • Which version of jQuery is this? It would be helpful if you could recreate the issues on jsbin or something. – Incognito Dec 01 '11 at 21:44

4 Answers4

6

To avoid that any newly created function (context) is closing over its parent scope here, you would just need to get rid of the anonymous function in setTimeout there. So

 setTimeout(self.timeoutHandler, 750);

However, even if that closure would clouse over the parent(s) contexts, any half decent garbage collector (like any modern browser have) will notice it and free the memory after the method has fired. The very important thing you didn't mention is on which browser you noticed the behavior. The Firefox garbage collector for instance, works .. pretty unpretictable (at least to me). It'll allow for more and more memory to get used and then on some point, it will release a huge chunk again.

To see what is going on, use Firefox and have a look into about:memory while your script is running. There you will see where the memory goes. I would not be worried if the memory usage increases for a while. Look at it, if that is all of your code, the memory should get freed sooner or later.

jAndy
  • 231,737
  • 57
  • 305
  • 359
  • Does this really work? If it does, please let me know and I'll have learned a new thing :) – Halcyon Dec 01 '11 at 21:20
  • @Nik: you need to remove the paranthesis. Just pass in the function reference `setTimeout (slf.timeoutHandler (), 750);` – jAndy Dec 01 '11 at 21:33
  • A good observation, though that is actually what I had in the code. – Nik Dec 01 '11 at 21:35
  • 1
    @Nik: in your updated code, you're creating an another anonymous function (context) for your `each` loop. This again would close over the parent context etc. One thing you might want to try is to entirely move out the `success` function to a another point. Like `success: mySuccessHandler` and then define that function outside. – jAndy Dec 01 '11 at 21:38
  • I will try that. Can you explain what that does for me? – Nik Dec 01 '11 at 21:40
  • @Nik: the only possible way, to "leak" memory in a way that the GC cannot free it is to hold references to objects. As long as there is a valid reference, it cannot get freed. Anytime a function is called, it'll copy the scopes of all parent context into its own "execution context". So basically my suggestion here is to avoid that. – jAndy Dec 01 '11 at 21:45
  • This hasn't made much of a change. It may have slowed a little. Based on the jumps in KB, I feel like my XML is being kept around. Is that just slow garbage collection, or are there lingering references? – Nik Dec 01 '11 at 21:50
  • @Nik: which browser are you testing on? You should watch the memory over a longer period, lets say, 10-15 minutes. Notice where the memory usage started and check back for that. – jAndy Dec 01 '11 at 21:52
  • In 10+ minutes on FF, I see an increase of 80%, but the increase isn't steady, there is some up and down. That will have to be good enough for now. – Nik Dec 01 '11 at 22:01
0

Your timeout function

setTimeout(self.timeoutHandler, 750);

may be causing memory leak. Although the success: callback is able to exit after finished, there may not be an empty context in a setTimeout callback causing a circular loop. Youl should take out the anonymous function to end up something like this:

var pManager = new PageManager ();
pManager.timeoutHandler();

function PageManager () {
var ret  = (function(self) { 
    return function() { self.timeoutHandler(); };
})(this);

this.timeoutHandler = function () {
    $.ajax ({
        url: '/echo/json/',
        type: 'post',
        cache: false,
        success: function (data) {
            setTimeout (ret, 750);
        }
    });
};
}

This should only call back once.

Tom Bell
  • 489
  • 2
  • 6
  • 15
0

Memory grows when you keep references to things. It's a leak when you accidentally do this.

In this case your function timeoutHandler calls itself, and again, and again ... It can never clean up the call stack. I'll bet this is your leak.

To avoid this problem use setInterval. It works exactly like setTimeout but it will keep calling the function every milliseconds until you clear it with clearTimeout (or until the world ends).

The down side is that you can't time it very well, your current implementation always waits 750 milliseconds after each call. I suppose you could do something fancy that will still allow you time it well (using Date to check the time), but it's not something I could write in 10 seconds :P

Halcyon
  • 57,230
  • 10
  • 89
  • 128
  • I didn't downvote you, but setInterval would be a bad idea. You don't know how long these requests are going to take, and so you could end up with the requests tripping over each other. With `setTimeout`, the next request doesn't begin until the previous is done. – RightSaidFred Dec 01 '11 at 21:13
  • Yes, that's what I said. The only problem is that you have to use `setInterval` because of the memory leak, Other than that, I completely agree with you ^^ . I'd be very interested to see the timing solution if you do get around to writing one. – Halcyon Dec 01 '11 at 21:17
  • But the `setTimeout` really shouldn't itself be causing a leak. Remember that both the AJAX call and the `setTimeout` are asynchronous. This means that both the `timeoutHandler` function as well as the `success:` callback are able to exit immediately after they're finished. When the `setTimeout` finally exits, there should be no more variable references from the initial `timeoutHandler` call. And because all the functions have exited, there's no more call stack (other than the new one created by the new `timeoutHandler` call), so the variables should be free to be collected. – RightSaidFred Dec 01 '11 at 21:21
  • Yes, but just because it's asynchronous doesn't mean you don't have a memory leak. You don't magically get a new empty context in a setTimeout callback (else you couldn't make closures \o/). At least, I assume you don't. I'd be amazed if jAndy's solution works. – Halcyon Dec 01 '11 at 21:25
  • When the `setTimeout` is invoked, it calls `self.timeoutHandler()`. Then `timeoutHandler()` makes the AJAX request, and exits immediately (not waiting for the AJAX response). This allows `timeoutHandler` to exit, which allows the `setTimeout` callback to exit, so there's nothing left to keep variables in play. When the `success:` callback is invoked, it does its work, calls the `setTimeout`, then exits, so there's nothing there to keep variables in play either. So I guess I just don't see where there would be a leak in the first place. – RightSaidFred Dec 01 '11 at 21:37
  • This has slowed my growth. It requires some extra checking to make sure I don't overlap requests, but the memory growth is about halved. – Nik Dec 01 '11 at 21:37
0

I'm just going to throw my two cents in but i agree jAndy (+1). However, I would generate a single bound callback once, rather than creating a new closure on ever iteration of the callback (which in theory could keep the scope and the xml data alive for a lot longer than you want. I would suggest another alternative like this:

function PageManager () {
    var callback  = (function(self) { 
        return function() { self.timeoutHandler(); };
    })(this); // only one callback

    this.timeoutHandler = function () {
        $.ajax ({
            url: '/echo/json/',
            type: 'post',
            cache: false,
            data: {a:Math.random() },
            success: function (data) {
                //var xmlDoc = $($.parseXML (data));
                // Processing of XML
                //alert("data");
                setTimeout (callback, 750);
            }
        });
    };
}

var pm = new PageManager ();
pm.timeoutHandler();

Edit

Here is a jsFiddle with the above code, and I watched the memory management for a little while, but not nearly enough to be conclusive in any sense.

J. Holmes
  • 18,466
  • 5
  • 47
  • 52