Resolved AJAX auto-refresh script memory leak problem.

[jon_hill987]

I have a memory leak[1] when using the following script. From the research I have done it seems likely to do with a circular reference meaning the xmlhttp request never gets cleaned up when it is finished with but I have no idea how to fix it.

function loadIndicators()
    {
        var xmlhttp;
       
        if (window.XMLHttpRequest)
            {// code for IE7+, Firefox, Chrome, Opera, Safari
                xmlhttp=new XMLHttpRequest();
            }
        else
            {// code for IE6, IE5
                xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
            }
           
        xmlhttp.onreadystatechange=function()
            {
            if (xmlhttp.readyState==4 && xmlhttp.status==200)
                {
                    xmlDoc=xmlhttp.responseXML;
                   
                //my code manipulating the page based on xmlDoc goes here, I call new arrays and variables and change the content of <div> elements but don't call anything else from outside this function except xmlhttp.
                   
                    setTimeout('loadIndicators()',500);
                                                 
                }           

            }
           
        xmlhttp.open("GET","indicators.xml?t=" + Math.random(),true); //loads the .xml file, the random number component is for IE compatibility, to stop it using cashed data.
        xmlhttp.send();
    }

Is there a better way of doing this? I am very new to javascript and this has been most cobbled together from various examples. If there is nothing wrong with this script I will post it again with my code added back in, I left it out for simplicity as the script comes to 350ish lines with it in.

Thanks,

Jon


[1] most noticeable in IE (8 and 9 tested), Chrome seems to have a problem too, nothing obvious in Firefox but I am going to leave that going overnight and see what happens.

[devnull69]

You see that you are creating a new XMLHttpRequest object with each and every call to loadIndicators() ? I guess that's the reason for your leak.

One more question: Do you really (really really) need to support IE6 and below? If not, you can forget about the browser switch. No ActiveX crap needed any more :-)

Third: It's always better to have a function reference as the first parameter to setTimeout rather than a string that needs to be evaluated first.

var xmlhttp = new XMLHttpRequest();

function loadIndicators()
    {
           
        xmlhttp.open("GET","indicators.xml?t=" + Math.random(),true); //loads the .xml file, the random number component is for IE compatibility, to stop it using cashed data.

        xmlhttp.onreadystatechange=function()
            {
            if (xmlhttp.readyState==4 && xmlhttp.status==200)
                {
                    xmlDoc=xmlhttp.responseXML;
                   
                // your code here
                   
                    setTimeout(loadIndicators, 500);
                                                 
                }           

            }

        xmlhttp.send();
    }

[jon_hill987]

Originally Posted by devnull69

You see that you are creating a new XMLHttpRequest object with each and every call to loadIndicators() ? I guess that's the reason for your leak.

Is there any way I can avoid that? The content of indicators.xml is being updated server side by a PLC so I do need to keep calling it again as far as I can tell, the idea is to have "live" data from the PLC displayed on the webpage. Oh and my test with Firefox showed that it can run all afternoon with no increase in memory usage. I also commented out my code section and tested with IE so the leak is definitely in the code I have posted, but I guess you knew that already.

One more question: Do you really (really really) need to support IE6 and below? If not, you can forget about the browser switch. No ActiveX crap needed any more :-)

Sadly, as this is to be run on industrial machines I have no guaranty of the browsers they will be able to use. , I figured that as it is behind an "if" statement it could not do any harm. I guess when it comes down to it I can leave it in or take it out on a case by case basis but unless it is going to cause issues I will leave it in for now.

Third: It's always better to have a function reference as the first parameter to setTimeout rather than a string that needs to be evaluated first.

I will make those couple changes you suggested. This code has been put together from various examples, mostly involving people that have more experience with programming PLCs (like myself) than they do with web design, which accounts for the poor practices... I did wonder why the "open" was after the main code, but since it worked I did not change it despite it not making sense.

Thanks for your help.

[devnull69]

Is there any way I can avoid that?

Yes, in my code example I put the code to create a new XMLHttpRequest object outside the function. That way it will not be called with every function call. As you still need the browser switch, you can do this

var xmlhttp;
       
if (window.XMLHttpRequest)
    {// code for IE7+, Firefox, Chrome, Opera, Safari
        xmlhttp=new XMLHttpRequest();
    }
else
    {// code for IE6, IE5
        xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
    }

function loadIndicators() {
  ...
}

[jon_hill987]

Originally Posted by devnull69

Yes, in my code example I put the code to create a new XMLHttpRequest object outside the function. That way it will not be called with every function call.

So you did, I missed that. :facepalm:
Re-reading your post now I see that you made that quite clear.

That has fixed the issue, no change in memory use whatsoever as I write this.

Thank you for your help, and your patience.


Jon