Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 5 of 5
  1. #1
    New to the CF scene
    Join Date
    Mar 2012
    Posts
    7
    Thanks
    1
    Thanked 0 Times in 0 Posts

    Question AJAX auto-refresh script memory leak problem.

    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.

    Code:
    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.
    Last edited by jon_hill987; 03-28-2012 at 11:44 AM.

  • #2
    Senior Coder
    Join Date
    Dec 2010
    Posts
    2,396
    Thanks
    12
    Thanked 569 Times in 562 Posts
    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.

    Code:
    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();
        }

  • #3
    New to the CF scene
    Join Date
    Mar 2012
    Posts
    7
    Thanks
    1
    Thanked 0 Times in 0 Posts
    Quote Originally Posted by devnull69 View Post
    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.
    Last edited by jon_hill987; 03-28-2012 at 09:42 AM.

  • #4
    Senior Coder
    Join Date
    Dec 2010
    Posts
    2,396
    Thanks
    12
    Thanked 569 Times in 562 Posts
    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
    Code:
    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() {
      ...
    }

  • Users who have thanked devnull69 for this post:

    jon_hill987 (03-28-2012)

  • #5
    New to the CF scene
    Join Date
    Mar 2012
    Posts
    7
    Thanks
    1
    Thanked 0 Times in 0 Posts

    Smile

    Quote Originally Posted by devnull69 View Post
    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
    Last edited by jon_hill987; 03-28-2012 at 11:56 AM.


  •  

    Tags for this Thread

    Posting Permissions

    • You may not post new threads
    • You may not post replies
    • You may not post attachments
    • You may not edit your posts
    •