A surprising JavaScript memory leak found at Meteor

Honestly, I&#x27;m not really sure I&#x27;d call this a &quot;memory leak&quot; -- I just always assumed that <i>all</i> the variables present in a closure are maintained, if there&#x27;s an existing reference to any function defined within. I&#x27;m not surprised at all, this is what I would expect.<p>I&#x27;m actually suprised&#x2F;impressed that Chrome has an optimization to detect which closure variables are actually used in a function and garbage-collect the rest.<p>But it&#x27;s certainly something important to be aware of. I think the main point is, why on earth would you be constantly re-defining a function &quot;logIt&quot; within a function that is repeatedly called? That&#x27;s just bad programming -- whenever you define functions within functions in JavaScript, you really need to know exactly what you&#x27;re doing.

TL;DR -- all closures created within a function are retained even if only one closure is ever referenced. This is really interesting, and potentially devastating for certain coding styles.<p>I ran the code on node and the RSS does appear to be increasing without bound. Even running node with --expose-gc and forcing a global.gc() inside logIt() causes the same unbounded memory growth.<p>Increasing the size of the array by a factor of 10 causes RSS usage to jump up by a factor of 10 every second, so we know that the memory usage isn&#x27;t caused by creating a new long-lived closure (i.e., the logIt() function) every second.<p>In fact, removing the call to doSomethingWithStr() doesn&#x27;t change the unbounded memory growth.<p>Here&#x27;s a shorter snippet that demonstrates the leak more dramatically (about 10 MB&#x2F;sec RSS growth):<p><pre><code> var run = function () { var str = new Array(10000000).join(&#x27;*&#x27;); var fn = function() { str; } var fn = function() { } setInterval(fn, 100); }; setInterval(run, 1000); </code></pre> Tried it out on node v0.8.18

OP here. Several people have pointed out that of course I should expect a leak, since each `logIt` object is leaked. That&#x27;s absolutely true; my point was just that we don&#x27;t want `str` to leak.<p>But the original bug that led to this discovery involved a data structure that shouldn&#x27;t have leaked at all. I&#x27;ve updated the post to show it; duplicated here since GitHub Pages seems to cache posts pretty aggressively.<p><pre><code> var theThing = null; var replaceThing = function () { var originalThing = theThing; &#x2F;&#x2F; Define a closure that references originalThing but doesn&#x27;t ever actually &#x2F;&#x2F; get called. But because this closure exists, originalThing will be in the &#x2F;&#x2F; lexical environment for all closures defined in replaceThing, instead of &#x2F;&#x2F; being optimized out of it. If you remove this function, there is no leak. var unused = function () { if (originalThing) console.log(&quot;hi&quot;); }; theThing = { longStr: new Array(1000000).join(&#x27;*&#x27;), &#x2F;&#x2F; While originalThing is theoretically accessible by this function, it &#x2F;&#x2F; obviously doesn&#x27;t use it. But because originalThing is part of the &#x2F;&#x2F; lexical environment, someMethod will hold a reference to originalThing, &#x2F;&#x2F; and so even though we are replacing theThing with something that has no &#x2F;&#x2F; effective way to reference the old value of theThing, the old value &#x2F;&#x2F; will never get cleaned up! someMethod: function () {} }; &#x2F;&#x2F; If you add `originalThing = null` here, there is no leak. }; setInterval(replaceThing, 1000);</code></pre>

An excellent catch. The parse tree for the closure should be able to ascertain the reachability of the variables in scope so that fences around particular sets of variables can be established. To do that would require something a bit more sophisticated than a single taint id though. You would really want a taint &#x27;flavor&#x27; such that for any closure and in variable in scope of that closure you could defined f(closure, var) which would return turn if that variable cannot be collected. I can&#x27;t think off the top of my head how you would recycle identifiers without risking temporal tainting (where a new closure in scope with the same id came at a later time and double tainted the variable leaving it essentially uncollectable or collected early. Hmm, or maybe not since their address on the heap will be different you&#x27;re probably ok with that.<p>FWIW that is certainly the twisty bits of garbage collected languages.

First, it is not a bug in JavaScript at all.<p>Here is a technical explanation of why there is a memory leak and how to fix the problem.<p>The scope chain of Closures (in JavaScript) contains the outer function(s) activation object. The activation object of a function contains all the variables that the function has access to, and it is part of a function’s scope chain.<p>This means the inner function (the closure) has access (a reference) to the outer function’s scope chain, including the global object. And even after the outer function has returned, the closure still has access to the outer function’s variables.<p>Therefore, the activation object of the outer function cannot be destroyed (for garbage collection) after it has returned, because the closure still references its variables.<p>When the outer function returns, its own scope chain for execution (its execution object) is destroyed, but its activation object is still referenced by the closure, so its variables will not be destroyed until the closure is destroyed.<p>The execution context of a function is associated with the functions’ activation object, but while the execution object is used for its own execution and is destroyed when it returns, its activation object is referenced by closures—its inner functions. 
<p>Now, as to the specific example in question: The reason the str variable is never destroyed is because it is referenced buy the logIt function because the logIt function&#x27;s execution object references the entire scope chain of the run function, and the logIt function is never destroyed, so the str variable remains in memory.<p>As the original author (OP) suggested, be sure to dereference any local variable in the outer function that the closure is using, once the closure is done with it or once the outer function is done with it.<p>Also, simply setting the logIt function to null (when it completes execution—returns) will allow the str variable and the entire scope of chain of both the logIt and the containing run function to be destroyed and ready for garbage collection.
<p>For a detailed explanation of closures in JavaScript, see &quot;Understand JavaScript Closures With Ease&quot;: <a href="http://javascriptissexy.com/understand-javascript-closures-with-ease/" rel="nofollow">http:&#x2F;&#x2F;javascriptissexy.com&#x2F;understand-javascript-closures-w...</a>

The C# compiler does the same thing:<p><a href="http://blogs.msdn.com/b/ericlippert/archive/2007/06/06/fyi-c-and-vb-closures-are-per-scope.aspx" rel="nofollow">http:&#x2F;&#x2F;blogs.msdn.com&#x2F;b&#x2F;ericlippert&#x2F;archive&#x2F;2007&#x2F;06&#x2F;06&#x2F;fyi-c...</a><p>This is an old article, but I believe it still applies to the latest iteration of the language.

I don&#x27;t know how to look at memory usage in other modern JS interpreters like those used in FireFox and Safari. Anyone able to check to see if they have this problem? (And if they manage to avoid the memory leaks in the first two code samples?)

It&#x27;s more of a surprising non-leak caused by clever GC that is insufficiently clever to handle pathological code. Interesting explanation, but yet another reason to be very careful of setInterval.

That this kind of issue exists in javascript should come as a surprise to no one - there are probably a huge number of these kind of issues just waiting to be found. It&#x27;s actually the key difference between a &#x27;designed&#x27; language and an &#x27;evolved&#x27; language.<p>EDIT: I find the down-votes very amusing. Has javascript now developed a cult? Read up on the history of javascript if my message above comes as a shock to you.

I don&#x27;t know how you can safely declare what&#x27;s used vs unused in a language where properties can be dynamically accessed.<p>It&#x27;s easy to check for dict.someProperty, but dict[propertyName] where propertyName can be the string &quot;someProperty&quot; is a much more complicated problem. Now you have to build a tree of everything that changes propertyName and make sure you know it can never be &quot;someProperty&quot;.

Nice find. I&#x27;ve run into some inexplicable garbage collection issues where canvas elements in an early version of flot ended up never getting released. I could figure out no way to clear these, but I remember the flot code made heavy use of closures. I should go revisit the code and see if this might explain it. I ended up simply hanging on to the elements and reusing them, had to patch flot to achieve this.

The fix in <a href="https://github.com/meteor/meteor/commit/49e9813" rel="nofollow">https:&#x2F;&#x2F;github.com&#x2F;meteor&#x2F;meteor&#x2F;commit&#x2F;49e9813</a> seems to be just limiting the effect of the problem, not removing it.<p>The variable is still being held in memory just with the value null instead of whatever it was before.<p>Or does setting it to null trigger some special GC algorithm that detects that it isn&#x27;t being used anymore?

This article taught me how to find a major leak in my Dart2js code so thanks John McCutchan &amp; Loreena Lee <a href="http://www.html5rocks.com/en/tutorials/memory/effectivemanagement/" rel="nofollow">http:&#x2F;&#x2F;www.html5rocks.com&#x2F;en&#x2F;tutorials&#x2F;memory&#x2F;effectivemanag...</a>

Looks like Go optimizes this fully, as pointed out by <a href="https://twitter.com/nynexrepublic/status/350717895971586049" rel="nofollow">https:&#x2F;&#x2F;twitter.com&#x2F;nynexrepublic&#x2F;status&#x2F;350717895971586049</a><p>If you run these on your own machine and peek at the RSIZE, it stays constant (well, the first grows slowly due to `logIt`).<p><a href="http://play.golang.org/p/A5Pz-3kthP" rel="nofollow">http:&#x2F;&#x2F;play.golang.org&#x2F;p&#x2F;A5Pz-3kthP</a> <a href="http://play.golang.org/p/RnXr_jB5Qh" rel="nofollow">http:&#x2F;&#x2F;play.golang.org&#x2F;p&#x2F;RnXr_jB5Qh</a>

&gt;<i>You could imagine a more clever implementation of lexical environments that avoids this problem. Each closure could have a dictionary containing only the variables which it actually reads and writes; the values in that dictionary would themselves be mutable cells that could be shared among the lexical environments of multiple closures.</i><p>That is basically how Lua implements closures.<p><a href="https://bugzilla.mozilla.org/show_bug.cgi?id=542074" rel="nofollow">https:&#x2F;&#x2F;bugzilla.mozilla.org&#x2F;show_bug.cgi?id=542074</a>

Kudos to the excellently talented Meteor team. A great product from a genuine customer (of a free product), who feels great that there is such attention to detail.

If we implement the suggested fix, and then someone evals code in that scope, they can get the surprising result that variables which look like they should be in scope actually aren&#x27;t. Careful analysis of this will have many edge cases to worry about.<p>Implementing the simplest thing that is correct according to the spec sounds like a good idea to me.

This problem has been encountered and studied before: see Appel and Zhao&#x27;s paper, Efficient and Safe-for-Space Closure Conversion.

The example code looks really silly. As others said, it&#x27;s just bad programming.

I&#x27;m surprised this comes as a surprise to Meteor developers. References retained won&#x27;t be garbage collected... regardless of whether the lexical scope is &quot;really small&quot; or not, lol.

That is surprising? A setInterval without a matching clearInterval is always a memory leak.