Javascript memory leak issue - promises & recursion

Question

I'm having memory issues with this piece of code:

var RequestManager = function(customRequestArgs){

    var requestManager = this;

    this.customRequestArgs = customRequestArgs              || [];



    this.CustomRequest = function(url, data){

        var requestDeferred = $.Deferred();

        // set default xmlRequestArgs
        var xmlRequestArgs = {
            method  : "GET",
            url     : url,
            onload  : function(response) {
                requestDeferred.resolve(response.responseText);
            },
            onerror : function(response){
                requestDeferred.reject('xmlRequest failed', response);
            }
        };
        // set custom xmlRequestArgs
        var i;
        for(i in requestManager.customRequestArgs){
            if(requestManager.customRequestArgs.hasOwnProperty(i)){
                xmlRequestArgs[i] = requestManager.customRequestArgs[i];
            }
        }

        // append data, depending on method
        var d = [];
        for(i in data){
            if(data.hasOwnProperty(i)){
                d.push(i+'='+encodeURIComponent(data[i]));
            }
        }
        var dataString = d.join('&');

        if(xmlRequestArgs.method.toLowerCase() === 'get'){
            if(url.indexOf('?')>=0){
                xmlRequestArgs.url = url+dataString;
            }
            else{
                xmlRequestArgs.url = url+'?'+dataString;
            }
        }
        if(xmlRequestArgs.method.toLowerCase() === 'post'){
            xmlRequestArgs.data = dataString;
        }


        // run request
        GM_xmlhttpRequest(xmlRequestArgs);

        return requestDeferred;
    };

    this.BatchRequestRunner = function(args){

        var maxParallelRequests = args.maxParallelRequests || 8;

        var onEachStart         = args.onEachStart              || function(requestIndex, url){return undefined;};          // must return undefined or loader promise (i.e. for cached results)
        var onEachSuccess       = args.onEachSuccess            || function(result, requestIndex, url){return result;};     // must return result or promise that resolves to result
        var onEachError         = args.onEachError              || function(error, requestIndex, url){return error;};       // must return error or promise that resolves to error

        var urlAr               = args.urlAr                    || [];

        var storeResults        = args.storeResults             || false;

        var reversedUrlArClone  = urlAr.slice(0).reverse();
        var deferredAr          = [];
        var resultAr            = [];
        var errorAr             = [];


        var runnerMethod = function(){

            if(reversedUrlArClone.length > 0){

                // get request url
                var url = reversedUrlArClone.pop();

                // get urlIndex (i-th url in urlAr)
                var requestIndex = urlAr.length - reversedUrlArClone.length - 1;


                // run onEachStart
                $.when(onEachStart(requestIndex, url)).then(function(loaderPromise){

                    if(loaderPromise === undefined){

                        // set loaderPromise
                        loaderPromise = requestManager.CustomRequest(url);

                    }

                    var generateOnSuccess = function(requestIndex){
                        return function(result){


                            $.when(onEachSuccess(result, requestIndex, url)).then(function(result){

                                // store result
                                if(storeResults){
                                    resultAr[requestIndex] = result;
                                }

                                // resolve deferredAr[requestIndex]
                                deferredAr[requestIndex].resolve();

                                // start runnerMethod for next request
                                runnerMethod();

                            });

                        };
                    };
                    var generateOnError = function(requestIndex){
                        return function(error){

                            $.when(onEachError(error, requestIndex, url)).then(function(error){

                                // store error
                                errorAr[requestIndex] = error;

                                // reject deferredAr[requestIndex]
                                deferredAr[requestIndex].reject();

                                // start runnerMethod for next request
                                runnerMethod(); 


                            });

                        };
                    };

                    // handle loader
                    loaderPromise.done(generateOnSuccess(requestIndex));
                    loaderPromise.fail(generateOnError(requestIndex));

                });

            }

        };

        var startParallelRequestThread = function(){
            runnerMethod();
        };

        var start = function(){
            var i,
                runnerDeferred  = $.Deferred();

            // setup deferredAr
            for(i=0;i<urlAr.length;i++){
                deferredAr.push($.Deferred());
            }

            // setup onSuccess
            $.when.apply($, deferredAr)
            .done(function(){
                runnerDeferred.resolve(resultAr);
            })
            // setup onError
            .fail(function(){
                runnerDeferred.reject(errorAr);
            });

            // start requestThreads
            for(i=0;i<maxParallelRequests;i++){
                startParallelRequestThread();
            }

            return runnerDeferred;
        };


        return {
            start       : start
        };

    };



    return {
        BatchRequestRunner  : this.BatchRequestRunner,
        CustomRequest       : this.CustomRequest,
    };
};

It should be a class to perform batch requests. The user has the ability to set default request parameters (additional headers etc) and a bunch of batch-settings.

While the code performs as expected, the browser crashes after a while. Checking the task manager shows me the tab's process eats up more and more memory. I've been trying to find the reason for this, but have been unable to. Anyone has any ideas please?

Please let me know if I can clearify anything.

Regards, klmdb

Answer 1

OK, I think I've got my mind round the code and it appears that you jump though a number of unneccessary hoops. The code can be greatly simplified chiefly through the use of two standard tricks :

• Use of $.extend() (in two places) which avoids the need to manually loop through objects.
• Use of Array.prototype.reduce() to transform an array into a .then() chain in lieu of "recursion".

Other features of the version below are :

• Results and errors are delivered via the promise chain, not accumulated in outer arrays.
• The need for requestIndex (in many places) disappears, as does the need for explicit closures for its maintenance.
• No Deferred objects are created, which should help make the executable less memory hungry.
• new is now optional when calling RequestManager(). The original code was ambiguous with regard to whether or not new was intended.

Here's the simplified version ...

var RequestManager = function(customRequestArgs) {
    var CustomRequest = function(url, data) {
        //GM_xmlhttpRequest is assumed to call $.ajax() (or one of its shorthand methods) and return a jqXHR object
        return GM_xmlhttpRequest($.extend({ //$.extend() replaces several lines of original code
            method: "GET",
            url: url,
            data: data
        }, customRequestArgs || {})).then(function(response) {
            return response.responseText;
        }, function(jqXHR, textStatus, errorThrown) {
            return ('xmlRequest failed: ' + textStatus);
        });
    };
    //Defaults are best defined (once per RequestManager) as an object, which can be extended with $.extend().
    var batchRequestDefaults = {
        maxParallelRequests: 8,
        onEachStart: function(url) { return undefined; }, // must return undefined or loader promise (i.e. for cached results)
        onEachSuccess: function(result, url){ return result; }, // must return result or promise that resolves to result
        onEachError: function(error, url){ return error; }, // must return error or promise that resolves to error.
        urlAr: [],
        storeResults: false
    };
    var BatchRequestRunner = function(args) {
        args = $.extend({}, batchRequestDefaults, args); //$.extend() replaces several lines of original code
        function runnerMethod(index, urlAr) {
            //Note recursion is avoided here by the use of .reduce() to build a flat .then() chain.
            return urlAr.reverse().reduce(function(promise, url) {
                var requestIndex = index++;
                return promise.then(function(result1) {
                    return $.when(args.onEachStart(requestIndex, url)).then(function(p) {
                        return (p === undefined) ? CustomRequest(url) : p;
                    }).then(function(result2) {
                        args.onEachSuccess(result2, requestIndex, url);
                        // No return value is necessary as result2 is assumed 
                        // to be fully handled by onEachSuccess(),
                        // so doesn't need to be passed down the promise chain.
                    }, function(error) {
                        // This is messy but : 
                        // (a) is consistent with the stated rules for writing onEachError() functions.
                        // (b) maintains the original code's behaviour of keeping going despite an error.
                        // This is achieved by returning a resolved promise from this error handler.
                        return $.when(args.onEachError(error, requestIndex, url)).then(function(error) {
                            return $.when(); //resolved promise
                        });
                    });
               });
            }, $.when());
        }
        var start = function() {
            // start requestThreads
            var i, promises = [],
                pitch = Math.ceil(args.urlAr / args.maxParallelRequests),
                startIndex, endIndex;
            for(i=0; i<args.maxParallelRequests; i++) {
                startIndex = pitch * i;
                endIndex = pitch * (i + 1) - 1;
                promises.push(runnerMethod(startIndex, args.urlAr.slice(startIndex, endIndex)));
            }
            // Note: Results and errors are assumed to be fully handled by onEachSuccess() and onEachError() so do not need to be handled here or passed on down the promise chain.
            return $.when.apply(null, promises);
        };
        return {
            start: start
        };
    };
    return {
        BatchRequestRunner: BatchRequestRunner,
        CustomRequest: CustomRequest
    };
};

untested so may well need debugging

The hardest aspect by far is the treatment of errors. The original code has rather odd behaviour in this regard, which I have tried to emulate through the use of faux (non-stopping) errors. Messy-messy but having purged the recursion, I can't think of another way to do it.

Barring errors on my part, the only difference in behaviour should be in the promise returned by start(), which will now deliver both a results array AND a (faux) errors array, bundled into a js plain object. This is consistent with runnerMethod keeping going despite errors.

Now that results are delivered via the promise chain, 'storeResults' has disappeared. I can't see any reason for ever wanting to run with anything other than storeResults === true.

My only(?) assumptions are that $ is jQuery and that GM_xmlhttpRequest employs jQuery.ajax() and returns (or can be made to return) its jqXHR object. This seems reasonable from what I can see. If the assumption is not valid, then you will need to revert that section of the code.

For further explanation see in-code comments.

When debugged, if it still crashes out, then I would suggest that it is just memory hungry rather than leaky per se.

EDIT

Having read (in comments below) descriptions of the batch process and onEachError() etc, start() and runnerMethod() have been edited above.

Summary of changes :

• Batch definition: start() now initiates its 8 parallel batches by passing slices of urlAr to runnerMethod().
• requestIndex: is reinstated in a very simple way.

The behaviour of the edited version is similar but not identical to that of the original code in the question. The difference is that each batch is predefined, not responsive.

Ultimately, removing the responsive behaviour may be a price worth paying if this version is less memory hungry and actually runs to completion, which is the object of the exercise.

To see the unedited code, see the question's edit history