views:

316

answers:

2

We have this anonymous function in our code, which is part of the jQuery's Ajax object parameters and which uses some variables from the function it is called from.

this.invoke = function(method, data, callback, error, bare) {
      $.ajax({
        success: function(res) {
            if (!callback) return;

            var result = "";
            if (res != null && res.length != 0)
                var result = JSON2.parse(res);

            if (bare)
            { callback(result); return; }

            for (var property in result) {
                callback(result[property]);
                break;
            }
        }
   });
}

I have omitted the extra code, but you get the idea. The code works perfectly fine, but it leaks 4 Kbs on each call in IE, so I want to refactor it to turn the anonymous function into a named one, like this.onSuccess = function(res) { .. }.

The problem is that this function uses variables from this.invoke(..), so I cannot just take it outside of its body. How do I correctly refactor this code, so that it does not use anonymous functions and parent function variables?

Update. I am thinking of creating a separate object, initializing it with the same parameters, and pass its onSuccess function as a parameter for jQuery's Ajax object. Although I suspect that it will still leak memory.

Update 2. I have found a few links suggesting that the actual leak might be caused by jQuery. http://stackoverflow.com/questions/2429056/simple-jquery-ajax-call-leaks-memory-in-ie http://stackoverflow.com/questions/1455947/memory-leak-involving-jquery-ajax-requests

Still it was good to find a way to refactor this.

Update 3. I will wait for a more generic solution, before accepting an answer.

+3  A: 

+1 for excellent, excellent question - I feel your pain - this is really nicely factored as it is.

One suggestion (and maybe this is what you meant by your update)...define a wrapper for onSuccess and make it return the function you want to assign. Then call the outer function and assign it to the "success" option, passing the values it needs. Those values will be pre-assigned to the variables in the inner function. Not actually sure if this will help - you still end up with an anonymous function - but worth a try

this.invoke = function(method, data, callback, error, bare) {
    $.ajax({
        success: onSuccess(callback, bare);
    });
};

var onSuccess = function(callback, bare) {
     return function() {
        if (!callback) return;

        var result = "";
        if (res != null && res.length != 0)
            var result = JSON2.parse(res);

        if (bare)
        { callback(result); return; }

        for (var property in result) {
            callback(result[property]);
            break;
        }
     }
}
plodder
This is a good way from a programming point of view, but it doesn't sort the leakage problem, as your anonymous function in return statement still references variables of the parent function.
HeavyWave
OK..as I feared. If its *really* the creation of anonymous functions after each ajax call that causes the memory leak then you are in a pickle. Not sure whats preventing garbage collection here - have you tried physically deleting the function when you are done with it?
plodder
+5  A: 

You can add extra params to the ajax request that can be accessed in the success callback:

this.invoke = function(method, data, callback, error, bare) {
    $.ajax({
        success: onSuccess,
        invokedata: {
         callback: callback,
         bare: bare
        }
    });
};

var onSuccess = function(res) {
    var callback = this.invokedata.callback,
        bare = this.invokedata.bare;
    if (!callback) return;

    var result = "";
    if (res != null && res.length != 0)
        var result = JSON2.parse(res);

    if (bare){
        callback(result); 
        return;
    }

    for (var property in result) {
        callback(result[property]);
        break;
    }
}
PetersenDidIt
Didn't know you could do that, thanks. I'll check to see if there is still a leak.
HeavyWave
how is res getting passed to onSuccess?
plodder
From jquery. http://api.jquery.com/jQuery.ajax/
HeavyWave
@Angus res is the data returned from the request
PetersenDidIt
ok thanks - I'm still unclear as to why IE is holding on to to the anonymous function. Sure the function refs some parent objects - but there should be no circular reference here - so I thought that tracking references top down from the window should not reach this function
plodder
@Angus Croll, See my update. The ajax call itself leaks, not the anonymous function in this case.
HeavyWave