This project is archived and is in readonly mode.

#1123 ✓ resolved
Luke Ehresman

memory leak with the Request class

Reported by Luke Ehresman | December 23rd, 2010 @ 07:58 PM | in 1.3.1 (closed)

I have discovered what I believe to be a pretty serious memory leak in the Request class. My site has long-running scripts with large data payloads. So, a user may leave the page open for days at a time without reloading the page. During that time, the site makes many AJAX requests to my server. I've noticed that my browser (all browsers I've tested -- Chrome, Safari, and Firefox) degrade pretty quickly and consume copious amounts of memory within a few hours.

After several days of debugging, I have identified that it's a problem with the Request class in MooTools. Here is a sample script to recreate the problem:

<!DOCTYPE html>
<html>
<head>
<script src="https://ajax.googleapis.com/ajax/libs/mootools/1.3.0/mootools.js"></script>
</head>
<body>
 
<script type="text/javascript">
    document.addEvent("domready", req);
 
    function req() {
        new Request({
            url: "/large_file.json", // a large JSON-encoded file, ~50k
            onSuccess: success
        }).get();
    }
 
    function success(resp) {
        setTimeout(req, 1000);
    }
</script>
 
</body>
</html>

I have set the timeout to 1 second in order to speed up the process and see the degradation more quickly. After just 1.5 minutes, here is the memory usage with MooTools (as monitored by the Chrome profiler): http://cl.ly/090p3k1Z3G26453l060N

For kicks, I ported this to jQuery too, just to see if it was something with my code. After 2.6 minutes, jQuery still does not show this pattern (http://cl.ly/3m363A1X1r3P2f232p0R). Here's the jQuery code:

<!DOCTYPE html>
<html>
<head>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.4/jquery.js"></script>
</head>
<body>
 
<script type="text/javascript">
    $(req);
 
    function req() {
        $.ajax({
            url: "/large_file.json", // a large JSON-encoded file, ~50k
            success: success
        });
    }
 
    function success(resp) {
        setTimeout(req, 1000);
    }
</script>
 
</body>
</html>

I have also implemented a solution using XHR directly, and it has even better performance memory-wise than jQuery: http://cl.ly/2E1i3f2H2T3b041S350S

<script type="text/javascript">
   document.addEvent("domready", req);

   function req() {
       var xhr = new Browser.Request();
       xhr.open('GET', '/large_file.json');
       xhr.onreadystatechange = function() {
           if (xhr.readyState != 4) return;
           setTimeout(req, 1000);
           delete xhr;
       };
       xhr.send();
   }

</script>

Comments and changes to this ticket

  • Arian

    Arian December 23rd, 2010 @ 08:11 PM

    • Milestone set to 1.3.1
    • State changed from “new” to “open”
    • Assigned user set to “Valerio”
    • Milestone order changed from “855” to “0”

    So the trick is to delete the xhr variable onreadystatechange?

  • Luke Ehresman

    Luke Ehresman December 23rd, 2010 @ 08:16 PM

    No, I think there's more to it than that. I tried modifying the MooTools code directly to delete the xhr variable and the same memory leak existed. I even trimmed the send() and other Request methods down to bare bones and still got the same leak. I'm not familiar enough with the MooTools internals to know exactly what's going on behind the scenes.

    It does appear that it has something to do with the payload being returned. Perhaps mootools is holding on to it somewhere (in a closure?) because my leak is directly proportional to the payload size of the response from the server.

  • seanmonstar

    seanmonstar December 23rd, 2010 @ 08:55 PM

    • Assigned user cleared.

    I started some initial debugging in a jsFiddle, and I didn't notice any significant memory gain from leaving it alone for several minutes. The only difference I can see is I wasn't requesting a heavy JSON file, just a simple json echo. I wonder if that's related?

    http://jsfiddle.net/seanmonstar/Jh8sf/5/

  • Luke Ehresman

    Luke Ehresman December 23rd, 2010 @ 09:44 PM

    Yeah, that's exactly what I noticed. This is hardly noticeable with small payloads. I suspect the leak is still there, but it's negligible. Try with a large payload (50k-100k) and monitor for a few minutes. That's when I started noticing the problem.

  • seanmonstar

    seanmonstar December 23rd, 2010 @ 09:55 PM

    I changed the the payload to a 20k file. I do see the memory increasing over time, but the Chrome profiler shows that a GC Event happens every 30ish seconds, and it seems to drop the memory back to around what it started at.

    http://jsfiddle.net/seanmonstar/Jh8sf/6/

  • Luke Ehresman

    Luke Ehresman December 24th, 2010 @ 12:09 AM

    Yes, but watch it over the course of a few minutes. The general trend is upward. There is something that the GC is not collecting.

  • Fábio M. Costa

    Fábio M. Costa December 27th, 2010 @ 12:34 AM

    lets try decreasing the context, Try the following code:

    document.addEvent("domready", req);
    
    var request = new Request({
       url: "/large_file.json", // a large JSON-encoded file, ~50k
       onSuccess: success
    });
    
    function req() {
       request.get();
    }
     
    function success(resp) {
       setTimeout(req, 1000);
    }
    

    I'm creating the request before and just using the send method when needed, like the Request class was designed to be used.

  • Christoph Pojer
  • Luke Ehresman

    Luke Ehresman January 11th, 2011 @ 01:33 AM

    I tried debugging the MooTools Request stuff myself, but never got anywhere. I ended up writing my own XHR request handler and am not using MooTools'. If I have time at some point, I'll try debugging MooTools' further, but we needed to ship our product out the door, so I couldn't spend any more time on it for now.

  • mius

    mius January 11th, 2011 @ 11:07 PM

    i added following lines to the onStateChange() of the request class:

    try {
        xhr.onreadystatechange = null;
        xhr.abort = null;
    } catch(e) {
        /* ie6 ignore */
    }
    
    xhr = null;
    

    that fixed the problem for me. see http://bugs.jquery.com/ticket/6242

  • Christoph Pojer

    Christoph Pojer February 23rd, 2011 @ 01:47 PM

    • State changed from “open” to “resolved”
    • Assigned user set to “Christoph Pojer”

    Hello,

    the above solution doesn't look like an appropriate fix to me.

    • setting onreadystatechange to null throws in IE6, therefore in your fix the second line in the try block is never being called even though...
    • abort is a native method, resetting it does not make sense
    • xhr = null does not have any side effects other than setting the xhr variable to null. It doesn't delete the xhr object (which is still being used in the success event etc.)

    This means that your code does not have any effect at all. Just not setting onreadystatechange to anything else than the original method seems to do the trick. Given that we always used a new function it is possible IE has some issues there. I now use the same function reference over and over again, therefore it should not allocate any new memory.

    Fix for 1.3.1: https://github.com/mootools/mootools-core/commit/cee4e5051f1843555f...

    I hope this fixes the leak :)

  • seanmonstar

    seanmonstar February 23rd, 2011 @ 05:06 PM

    The leak was in all browsers, not just IE6.

  • Alfredo Artiles

    Alfredo Artiles March 6th, 2011 @ 03:30 PM

    I have the same problem on Adobe Air 2.5 and Mootools 1.3.1 didn't fix it.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Attachments

Referenced by

Pages