11 ms·

A surprising JavaScript memory leak found at Meteor

179 points by glasser 13y ago

btipling 13y ago
Nice find. I'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.
33a 13y ago
That is surprising? A setInterval without a matching clearInterval is always a memory leak.
- byroot 13y ago
  No it's not. setInteval(function() { console.log('foo'); }, 1000) Is not a memory leak. But I agree that it's not surprising at all. If you can access a variable from somewhere it will obviously not be garbage collected.
  - mayank 13y ago
    The point is that you can't access str from elsewhere, which is why it is surprising.
    - oddthink 13y ago
      But you're still clearly retaining a reference to the lexical frame, so I'd think the "zeroth-order" assumption would be that you'd be keeping everything in that frame. It's nice that JavaScript will do enough tree-shaking that it will eliminate the single-closure case, but that's an optimization, not something you should be counting on. Similarly, I bet if you had a call to "eval" in there, you'd keep the entire frame, but I don't understand the exact semantics of JavaScript's eval enough to really say. I suppose this boils down to whether your mental model is more "frame-like" or more "upvalue-like". If you think of keeping around the containing frame, it's not at all surprising that you retain the variable. If you think of the closure as a discrete set of upvalues, then it's a surprise.
      - mayank 13y ago
        > I'd think the "zeroth-order" assumption would be that you'd be keeping everything in that frame. Why would that be a reasonable assumption? If a variable cannot ever be referenced, it should be garbage collected. In the example, "str" cannot ever be referenced. This is why we use garbage collected languages. > but that's an optimization, not something you should be counting on. It's not an optimization, it's correct GC behavior, and it's absolutely something you should be counting on if you're using a GC language.
        oddthink 13y ago
        I suppose I don't find it surprising, because I've used a lot of R, and R can't even optimize the simple case. There, it's for good reason, since you can extract the environment of any function, so once a closure escapes, you have a handle onto its entire containing frame. I think I've used scheme implementations that have the same "feature", with less of an excuse. It's a similar "failure of GC" if you create an object with two fields, like "obj={a: giant_value, b: 5}" then had your closure return obj.b. I bet the "a" slot would not be GC'd, even though there's no way to reach it. If your mental model is that you're holding onto the lexical frame, it's a pretty much the same behavior.
        mayank 13y ago
        > like "obj={a: giant_value, b: 5}" then had your closure return obj.b. I bet the "a" slot would not be GC'd, even though there's no way to reach it. It absolutely should be GC'd. To demonstrate (on node again), this uses about 10MB of RSS, because obj.a cannot be GC'd. j = function() { var obj = { a: new Array(1000000).join('*'), b: 5 }; return obj; }(); setTimeout(function() {}, 30000); If you change "return obj" to "return obj.b", RSS is about 9kb. Edit: I think your understanding of GC might be coming from R's generational GC, which is a heuristic and quite different from V8's incremental GC: http://blog.chromium.org/2011/11/game-changer-for-interactive.html http://blog.chromium.org/2011/11/game-changer-for-interactiv...
        13y ago
  - 33a 13y ago
    That allocates a closure. Now if you want it to sit around forever, then hey it is working as intended. Otherwise, it does "leak" some memory that will never get reclaimed by the GC.
- glasser 13y ago
  The surprise isn't that the memory associated with `logIt` is leaked. The surprise is that `logIt` holds on to the giant `str` object in its environment, despite the fact that there is literally no way for the `str` variable to ever be used again. V8 is smart enough to not make `logIt` hold on to `str` if there are no closures at all which refer to `str`. It's the introduction of the unrelated `doSomethingWithStr` closure that forces `str` into the lexical environment.
  - glasser 13y ago
    BTW, the original bug in the Meteor codebase wasn't a setInterval. It was that code to replace a certain type of object (a Spark renderer) with a new instance of that object accidentally ended up with a reference to the preceding renderer in a closure assigned somewhere on it, even though that particular closure didn't actually use that reference. So instead of replacing renderer #N with renderer #(N+1) and GCing #N, we ended up with a stream of renderers which never could be GCed... even though the reference keeping the old ones alive was literally impossible to ever use.
mayank 13y ago
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. 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. 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't caused by creating a new long-lived closure (i.e., the logIt() function) every second. In fact, removing the call to doSomethingWithStr() doesn't change the unbounded memory growth. Here's a shorter snippet that demonstrates the leak more dramatically (about 10 MB/sec RSS growth): var run = function () { var str = new Array(10000000).join('*'); var fn = function() { str; } var fn = function() { } setInterval(fn, 100); }; setInterval(run, 1000); Tried it out on node v0.8.18
- glasser 13y ago
  To be clear: when you say "removing the call to doSomethingWithStr() doesn't change the unbounded memory growth" you do literally mean "removing the call" and not "removing the function definition", right? That matches what I see.
  - mayank 13y ago
    Correct. I'm actually a little surprised that an identity statement like "str;" isn't optimized out. Edit: removing the function definition eliminates the problem, so I'd say that your diagnosis of the problem is spot on.
- richardofyork 13y ago
  >> 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. << This is not devastating, it is how closures and scope chain work in JavaScript. See my detailed explanation below. The crux of the matter is that the closure still references the outer function's scope chain, even after the outer function has returned. And the outer functions's scope chain is referenced by the closure until the closure is destroyed or returns.
ChuckMcM 13y ago
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 'flavor' 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'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're probably ok with that. FWIW that is certainly the twisty bits of garbage collected languages.
- pcwalton 13y ago
  > 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. Sadly it's not quite that simple because of the behavior of `eval` (strictly speaking, direct `eval`): it can leak bindings into the surrounding scope.
  - j_s 13y ago
    Nice! You've explained why this 'memory leak' could be by design - blame eval. In fact, Chrome GC-ing unreferenced variables could break eval if they didn't cover that case.
    - pcwalton 13y ago
      Only direct `eval` though. As long as you don't see any calls to symbols named `eval`, you can implement this optimization. See http://stackoverflow.com/questions/8694300/how-eval-in-javascript-changes-the-calling-context http://stackoverflow.com/questions/8694300/how-eval-in-javas...
      - deleted 13y ago
glasser 13y ago
I don'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?)
- shardling 13y ago
  In Firefox you could always do it manually with about:memory -- 1MB a second should be easy enough to notice. Not sure they've added a way to get a pretty graph yet, though. Probably Firebug has a way? I wonder if using the "use strict" directive would let the browser optimize this more easily? Probably not, but Javascript does have a lot of crazy corner cases, and one of them might be preventing (or just making it harder to prove safe) the optimization. e: Here's an example of one of those corner cases: console.log = eval Now whether those functions reference a particular variable depends on the string they're printing! And this swap out of log() could be done at any time. e2: Ah, as glasser and pcwalton point out, an indirect reference to eval doesn't work the same way as a direct call. TIL!
  - glasser 13y ago
    No, actually, you literally have to call eval as a function called eval if you want to get the local lexicals. See sections 10.4.2 and 15.1.2.1.1 of the ECMAScript standard, or try it out: > (function () { var x = 5; eval("console.log(x)"); })() 5 > (function () { var x = 5; var e = eval; e("console.log(x)"); })() ReferenceError: x is not defined
- masklinn 13y ago
  I haven't checked the first two samples but: * Safari 5.1 exhibits no significant heap growth, as far as the timeline shows anyway * Using top to observe private memory, Firefox does seem experience significant heap growth on the provided program
RyanZAG 13y ago
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's actually the key difference between a 'designed' language and an 'evolved' language. 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.
- nknighthb 13y ago
  I hate JavaScript myself, but you're blaming it for what is actually an implementation problem, not a property of the language itself. It's also a problem I could see occurring in the implementation of any other language that supports closures.
- lquist 13y ago
  I wonder if this will temper the rise of the server-side JS frameworks?
  - RyanZAG 13y ago
    Not likely - these kind of bugs are generally fixed when found, or remain hidden simply because that part of the language is very rarely used. You'll likely see this bug in Chrome fixed within a few months. I was commenting more on the practical effects of having an evolved language - this is a known and likely outcome in an evolved language. Designed languages generally spend a lot of time trying to avoid these kind of issues - it makes sense that not spending that time and adding in features as they are thought up means issues like this will creep in somewhere.
    - pcwalton 13y ago
      The only language features at play here are closures and garbage collection. Pick your favorite "designed" language: if it has those two features, an implementation of that language could have this bug.
  - PommeDeTerre 13y ago
crazygringo 13y ago
Honestly, I'm not really sure I'd call this a "memory leak" -- I just always assumed that all the variables present in a closure are maintained, if there's an existing reference to any function defined within. I'm not surprised at all, this is what I would expect. I'm actually suprised/impressed that Chrome has an optimization to detect which closure variables are actually used in a function and garbage-collect the rest. But it'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 "logIt" within a function that is repeatedly called? That's just bad programming -- whenever you define functions within functions in JavaScript, you really need to know exactly what you're doing.
- tolmasky 13y ago
  > why on earth would you be constantly re-defining a function "logIt" within a function that is repeatedly called? That's just bad programming -- whenever you define functions within functions in JavaScript, you really need to know exactly what you're doing. It's just a reduction to demonstrate the problem. I'm sure the code that actually triggered it was much more complex and had valid reasons for doing whatever it was doing.
  - crazygringo 13y ago
    I'm sure it was more complex, of course. But what I mean is -- whenever you define a function within another function, that should never be a "default" way of writing JavaScript, because it creates closures, and you're just asking for "memory leaks". Because closures use memory, you need to make sure there's a valid reason for creating the closure, and above all that if you're creating multiple closures by calling the parent function repeatedly, that there's a really really good reason for it, and that the closures are able to be garbage-collected later on.
    - jcheng 13y ago
      Hmmm, idiomatic Node.js code creates closures everywhere. (And you may be right that this is just asking for memory leaks.)
wingspan 13y ago
The C# compiler does the same thing: http://blogs.msdn.com/b/ericlippert/archive/2007/06/06/fyi-c-and-vb-closures-are-per-scope.aspx http://blogs.msdn.com/b/ericlippert/archive/2007/06/06/fyi-c... This is an old article, but I believe it still applies to the latest iteration of the language.
- RyanZAG 13y ago
  Does indeed still apply, and is actually formalized into the language spec itself [1] in section 7.15.5.1 Captured outer variables. So it's actually a feature (really) in the case of C#, not a bug. [1] http://www.microsoft.com/en-us/download/details.aspx?id=7029 http://www.microsoft.com/en-us/download/details.aspx?id=7029 "the local variable x is captured by the anonymous function, and the lifetime of x is extended at least until the delegate returned from F becomes eligible for garbage collection (which doesn’t happen until the very end of the program)."
  - br1 13y ago
    I don't see how that part of standard mandates that all anomymous functions share the same closure. The example only has a single lambda.
dreamdu5t 13y ago
I'm surprised this comes as a surprise to Meteor developers. References retained won't be garbage collected... regardless of whether the lexical scope is "really small" or not, lol.
- wtbob 13y ago
  The surprise is that a non-referenced value isn't collected, not that a referred-to value isn't.
mAritz 13y ago
The fix in https://github.com/meteor/meteor/commit/49e9813 https://github.com/meteor/meteor/commit/49e9813 seems to be just limiting the effect of the problem, not removing it. The variable is still being held in memory just with the value null instead of whatever it was before. Or does setting it to null trigger some special GC algorithm that detects that it isn't being used anymore?
- mistercow 13y ago
  There's nothing particularly special about null - it's really just that it's being set equal to any constant (or object that already exists for the life of the program). The object that was allocated and previously held in renderer will be GC'd (nothing special about that - it doesn't have any references anymore), and setting to null doesn't create a new allocation. You still have "renderer" as an entry in the dictionary of variables for the closure (which takes up a small amount of memory, presumably), but it's no longer preventing an object from being deallocated.
bsimpson 13y ago
I don't know how you can safely declare what's used vs unused in a language where properties can be dynamically accessed. It's easy to check for dict.someProperty, but dict[propertyName] where propertyName can be the string "someProperty" 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 "someProperty".
- masklinn 13y ago
  You can't dynamically access scope objects in standard javascript[0] except by using `eval`, and `eval` already triggers special handling and deoptimizations in pretty much all browsers. [0] outside of the global scope through `window` but all variables are clearly local here so that doesn't apply.
  - bsimpson 13y ago
    Doh! Good point.
michaelwww 13y ago
This article taught me how to find a major leak in my Dart2js code so thanks John McCutchan & Loreena Lee http://www.html5rocks.com/en/tutorials/memory/effectivemanagement/ http://www.html5rocks.com/en/tutorials/memory/effectivemanag...
Tloewald 13y ago
It'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.
- ufo 13y ago
  Not only setInterval though. This sort of leak can also show up if the closure is a DOM event handler or is otherwise exposed.
  - Tloewald 13y ago
    It can show up anywhere — and in fact event handlers are a major source of leaks in general — but the point is that the leak doesn't occur as frequently as you might expect not that it occurs.
glasser 13y ago
OP here. Several people have pointed out that of course I should expect a leak, since each `logIt` object is leaked. That's absolutely true; my point was just that we don't want `str` to leak. But the original bug that led to this discovery involved a data structure that shouldn't have leaked at all. I've updated the post to show it; duplicated here since GitHub Pages seems to cache posts pretty aggressively. var theThing = null; var replaceThing = function () { var originalThing = theThing; // Define a closure that references originalThing but doesn't ever actually // get called. But because this closure exists, originalThing will be in the // lexical environment for all closures defined in replaceThing, instead of // being optimized out of it. If you remove this function, there is no leak. var unused = function () { if (originalThing) console.log("hi"); }; theThing = { longStr: new Array(1000000).join('*'), // While originalThing is theoretically accessible by this function, it // obviously doesn't use it. But because originalThing is part of the // lexical environment, someMethod will hold a reference to originalThing, // and so even though we are replacing theThing with something that has no // effective way to reference the old value of theThing, the old value // will never get cleaned up! someMethod: function () {} }; // If you add `originalThing = null` here, there is no leak. }; setInterval(replaceThing, 1000);
richardofyork 13y ago
First, it is not a bug in JavaScript at all. Here is a technical explanation of why there is a memory leak and how to fix the problem. 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. 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. 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. 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. 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.   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'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. 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. 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.  For a detailed explanation of closures in JavaScript, see "Understand JavaScript Closures With Ease": http://javascriptissexy.com/understand-javascript-closures-with-ease/ http://javascriptissexy.com/understand-javascript-closures-w...
- nknighthb 13y ago
  You seem to be describing behavior that the article did not. Specifically, the retention of 'str' because of the 'logIt' function. If you read more closely, you'll notice that logIt was not the cause of 'str's retention, but instead, the doSomethingWithStr function was. When logIt is present by itself, str is not retained. Only when doSomethingWithStr is added alongside logIt is str retained.
btilly 13y ago
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't. Careful analysis of this will have many edge cases to worry about. Implementing the simplest thing that is correct according to the spec sounds like a good idea to me.
- glasser 13y ago
  Yes, but you can statically determine whether or not eval is called. A world-class JavaScript environment like V8 is far past "the simplest thing that is correct according to the spec". In fact, the JavaScript spec doesn't actually define anything about garbage collection! You can create a compliant ECMAScript-262 runtime that literally never collects any garbage ever. That's certainly the simplest correct thing. But it's a bad idea! V8 already goes to the trouble of figuring out whether or not a given variable needs to be stored in the lexical environment or not. Specifically: variables that are not used in ANY closures and where there is no eval in sight can be stored outside of the lexical environment. This is great, and better than many similar programming language environments offer. It just would be even better if they went one step farther.
  - btilly 13y ago
    V8 attempts to be fast. There is a trade-off between sophisticated analysis during compilation, and page performance. I'm not as convinced as you that they haven't found the right balance.
glasser 13y ago
Looks like Go optimizes this fully, as pointed out by https://twitter.com/nynexrepublic/status/350717895971586049 https://twitter.com/nynexrepublic/status/350717895971586049 If you run these on your own machine and peek at the RSIZE, it stays constant (well, the first grows slowly due to `logIt`). http://play.golang.org/p/A5Pz-3kthP http://play.golang.org/p/A5Pz-3kthP http://play.golang.org/p/RnXr_jB5Qh http://play.golang.org/p/RnXr_jB5Qh
finnw 13y ago
>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. That is basically how Lua implements closures. https://bugzilla.mozilla.org/show_bug.cgi?id=542074 https://bugzilla.mozilla.org/show_bug.cgi?id=542074
doctorpangloss 13y ago
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.
oakaz 13y ago
The example code looks really silly. As others said, it's just bad programming.
deleted 13y ago
[deleted]
gsg 13y ago
This problem has been encountered and studied before: see Appel and Zhao's paper, Efficient and Safe-for-Space Closure Conversion.
- glasser 13y ago
  Cool! A major reason I made this post was because I assumed that this must be something people had seen before, but I had trouble finding any references to it in the JavaScript context. I'll add a link to http://flint.cs.yale.edu/flint/publications/escc.pdf http://flint.cs.yale.edu/flint/publications/escc.pdf to the blog post!

179 points by glasser 13y ago

Nice find. I'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.

33a 13y ago

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

byroot 13y ago
No it's not. setInteval(function() { console.log('foo'); }, 1000) Is not a memory leak. But I agree that it's not surprising at all. If you can access a variable from somewhere it will obviously not be garbage collected.
- mayank 13y ago
  The point is that you can't access str from elsewhere, which is why it is surprising.
  - oddthink 13y ago
    But you're still clearly retaining a reference to the lexical frame, so I'd think the "zeroth-order" assumption would be that you'd be keeping everything in that frame. It's nice that JavaScript will do enough tree-shaking that it will eliminate the single-closure case, but that's an optimization, not something you should be counting on. Similarly, I bet if you had a call to "eval" in there, you'd keep the entire frame, but I don't understand the exact semantics of JavaScript's eval enough to really say. I suppose this boils down to whether your mental model is more "frame-like" or more "upvalue-like". If you think of keeping around the containing frame, it's not at all surprising that you retain the variable. If you think of the closure as a discrete set of upvalues, then it's a surprise.
    - mayank 13y ago
      > I'd think the "zeroth-order" assumption would be that you'd be keeping everything in that frame. Why would that be a reasonable assumption? If a variable cannot ever be referenced, it should be garbage collected. In the example, "str" cannot ever be referenced. This is why we use garbage collected languages. > but that's an optimization, not something you should be counting on. It's not an optimization, it's correct GC behavior, and it's absolutely something you should be counting on if you're using a GC language.
      - oddthink 13y ago
        I suppose I don't find it surprising, because I've used a lot of R, and R can't even optimize the simple case. There, it's for good reason, since you can extract the environment of any function, so once a closure escapes, you have a handle onto its entire containing frame. I think I've used scheme implementations that have the same "feature", with less of an excuse. It's a similar "failure of GC" if you create an object with two fields, like "obj={a: giant_value, b: 5}" then had your closure return obj.b. I bet the "a" slot would not be GC'd, even though there's no way to reach it. If your mental model is that you're holding onto the lexical frame, it's a pretty much the same behavior.
        mayank 13y ago
        > like "obj={a: giant_value, b: 5}" then had your closure return obj.b. I bet the "a" slot would not be GC'd, even though there's no way to reach it. It absolutely should be GC'd. To demonstrate (on node again), this uses about 10MB of RSS, because obj.a cannot be GC'd. j = function() { var obj = { a: new Array(1000000).join('*'), b: 5 }; return obj; }(); setTimeout(function() {}, 30000); If you change "return obj" to "return obj.b", RSS is about 9kb. Edit: I think your understanding of GC might be coming from R's generational GC, which is a heuristic and quite different from V8's incremental GC: http://blog.chromium.org/2011/11/game-changer-for-interactive.html http://blog.chromium.org/2011/11/game-changer-for-interactiv...
        13y ago
- 33a 13y ago
  That allocates a closure. Now if you want it to sit around forever, then hey it is working as intended. Otherwise, it does "leak" some memory that will never get reclaimed by the GC.
glasser 13y ago
The surprise isn't that the memory associated with `logIt` is leaked. The surprise is that `logIt` holds on to the giant `str` object in its environment, despite the fact that there is literally no way for the `str` variable to ever be used again. V8 is smart enough to not make `logIt` hold on to `str` if there are no closures at all which refer to `str`. It's the introduction of the unrelated `doSomethingWithStr` closure that forces `str` into the lexical environment.
- glasser 13y ago
  BTW, the original bug in the Meteor codebase wasn't a setInterval. It was that code to replace a certain type of object (a Spark renderer) with a new instance of that object accidentally ended up with a reference to the preceding renderer in a closure assigned somewhere on it, even though that particular closure didn't actually use that reference. So instead of replacing renderer #N with renderer #(N+1) and GCing #N, we ended up with a stream of renderers which never could be GCed... even though the reference keeping the old ones alive was literally impossible to ever use.

mayank 13y ago

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. 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. 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't caused by creating a new long-lived closure (i.e., the logIt() function) every second. In fact, removing the call to doSomethingWithStr() doesn't change the unbounded memory growth. Here's a shorter snippet that demonstrates the leak more dramatically (about 10 MB/sec RSS growth): var run = function () { var str = new Array(10000000).join('*'); var fn = function() { str; } var fn = function() { } setInterval(fn, 100); }; setInterval(run, 1000); Tried it out on node v0.8.18

glasser 13y ago
To be clear: when you say "removing the call to doSomethingWithStr() doesn't change the unbounded memory growth" you do literally mean "removing the call" and not "removing the function definition", right? That matches what I see.
- mayank 13y ago
  Correct. I'm actually a little surprised that an identity statement like "str;" isn't optimized out. Edit: removing the function definition eliminates the problem, so I'd say that your diagnosis of the problem is spot on.
richardofyork 13y ago
>> 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. << This is not devastating, it is how closures and scope chain work in JavaScript. See my detailed explanation below. The crux of the matter is that the closure still references the outer function's scope chain, even after the outer function has returned. And the outer functions's scope chain is referenced by the closure until the closure is destroyed or returns.

ChuckMcM 13y ago

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 'flavor' 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'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're probably ok with that. FWIW that is certainly the twisty bits of garbage collected languages.

pcwalton 13y ago
> 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. Sadly it's not quite that simple because of the behavior of `eval` (strictly speaking, direct `eval`): it can leak bindings into the surrounding scope.
- j_s 13y ago
  Nice! You've explained why this 'memory leak' could be by design - blame eval. In fact, Chrome GC-ing unreferenced variables could break eval if they didn't cover that case.
  - pcwalton 13y ago
    Only direct `eval` though. As long as you don't see any calls to symbols named `eval`, you can implement this optimization. See http://stackoverflow.com/questions/8694300/how-eval-in-javascript-changes-the-calling-context http://stackoverflow.com/questions/8694300/how-eval-in-javas...
    - deleted 13y ago

glasser 13y ago

I don'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?)

shardling 13y ago
In Firefox you could always do it manually with about:memory -- 1MB a second should be easy enough to notice. Not sure they've added a way to get a pretty graph yet, though. Probably Firebug has a way? I wonder if using the "use strict" directive would let the browser optimize this more easily? Probably not, but Javascript does have a lot of crazy corner cases, and one of them might be preventing (or just making it harder to prove safe) the optimization. e: Here's an example of one of those corner cases: console.log = eval Now whether those functions reference a particular variable depends on the string they're printing! And this swap out of log() could be done at any time. e2: Ah, as glasser and pcwalton point out, an indirect reference to eval doesn't work the same way as a direct call. TIL!
- glasser 13y ago
  No, actually, you literally have to call eval as a function called eval if you want to get the local lexicals. See sections 10.4.2 and 15.1.2.1.1 of the ECMAScript standard, or try it out: > (function () { var x = 5; eval("console.log(x)"); })() 5 > (function () { var x = 5; var e = eval; e("console.log(x)"); })() ReferenceError: x is not defined
masklinn 13y ago
I haven't checked the first two samples but: * Safari 5.1 exhibits no significant heap growth, as far as the timeline shows anyway * Using top to observe private memory, Firefox does seem experience significant heap growth on the provided program

RyanZAG 13y ago

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's actually the key difference between a 'designed' language and an 'evolved' language. 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.

nknighthb 13y ago
I hate JavaScript myself, but you're blaming it for what is actually an implementation problem, not a property of the language itself. It's also a problem I could see occurring in the implementation of any other language that supports closures.
lquist 13y ago
I wonder if this will temper the rise of the server-side JS frameworks?
- RyanZAG 13y ago
  Not likely - these kind of bugs are generally fixed when found, or remain hidden simply because that part of the language is very rarely used. You'll likely see this bug in Chrome fixed within a few months. I was commenting more on the practical effects of having an evolved language - this is a known and likely outcome in an evolved language. Designed languages generally spend a lot of time trying to avoid these kind of issues - it makes sense that not spending that time and adding in features as they are thought up means issues like this will creep in somewhere.
  - pcwalton 13y ago
    The only language features at play here are closures and garbage collection. Pick your favorite "designed" language: if it has those two features, an implementation of that language could have this bug.
- PommeDeTerre 13y ago

crazygringo 13y ago

Honestly, I'm not really sure I'd call this a "memory leak" -- I just always assumed that all the variables present in a closure are maintained, if there's an existing reference to any function defined within. I'm not surprised at all, this is what I would expect. I'm actually suprised/impressed that Chrome has an optimization to detect which closure variables are actually used in a function and garbage-collect the rest. But it'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 "logIt" within a function that is repeatedly called? That's just bad programming -- whenever you define functions within functions in JavaScript, you really need to know exactly what you're doing.

tolmasky 13y ago
> why on earth would you be constantly re-defining a function "logIt" within a function that is repeatedly called? That's just bad programming -- whenever you define functions within functions in JavaScript, you really need to know exactly what you're doing. It's just a reduction to demonstrate the problem. I'm sure the code that actually triggered it was much more complex and had valid reasons for doing whatever it was doing.
- crazygringo 13y ago
  I'm sure it was more complex, of course. But what I mean is -- whenever you define a function within another function, that should never be a "default" way of writing JavaScript, because it creates closures, and you're just asking for "memory leaks". Because closures use memory, you need to make sure there's a valid reason for creating the closure, and above all that if you're creating multiple closures by calling the parent function repeatedly, that there's a really really good reason for it, and that the closures are able to be garbage-collected later on.
  - jcheng 13y ago
    Hmmm, idiomatic Node.js code creates closures everywhere. (And you may be right that this is just asking for memory leaks.)

wingspan 13y ago

The C# compiler does the same thing: http://blogs.msdn.com/b/ericlippert/archive/2007/06/06/fyi-c-and-vb-closures-are-per-scope.aspx http://blogs.msdn.com/b/ericlippert/archive/2007/06/06/fyi-c... This is an old article, but I believe it still applies to the latest iteration of the language.

RyanZAG 13y ago
Does indeed still apply, and is actually formalized into the language spec itself [1] in section 7.15.5.1 Captured outer variables. So it's actually a feature (really) in the case of C#, not a bug. [1] http://www.microsoft.com/en-us/download/details.aspx?id=7029 http://www.microsoft.com/en-us/download/details.aspx?id=7029 "the local variable x is captured by the anonymous function, and the lifetime of x is extended at least until the delegate returned from F becomes eligible for garbage collection (which doesn’t happen until the very end of the program)."
- br1 13y ago
  I don't see how that part of standard mandates that all anomymous functions share the same closure. The example only has a single lambda.

dreamdu5t 13y ago

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

wtbob 13y ago
The surprise is that a non-referenced value isn't collected, not that a referred-to value isn't.

mAritz 13y ago

The fix in https://github.com/meteor/meteor/commit/49e9813 https://github.com/meteor/meteor/commit/49e9813 seems to be just limiting the effect of the problem, not removing it. The variable is still being held in memory just with the value null instead of whatever it was before. Or does setting it to null trigger some special GC algorithm that detects that it isn't being used anymore?

mistercow 13y ago
There's nothing particularly special about null - it's really just that it's being set equal to any constant (or object that already exists for the life of the program). The object that was allocated and previously held in renderer will be GC'd (nothing special about that - it doesn't have any references anymore), and setting to null doesn't create a new allocation. You still have "renderer" as an entry in the dictionary of variables for the closure (which takes up a small amount of memory, presumably), but it's no longer preventing an object from being deallocated.

bsimpson 13y ago

I don't know how you can safely declare what's used vs unused in a language where properties can be dynamically accessed. It's easy to check for dict.someProperty, but dict[propertyName] where propertyName can be the string "someProperty" 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 "someProperty".

masklinn 13y ago
You can't dynamically access scope objects in standard javascript[0] except by using `eval`, and `eval` already triggers special handling and deoptimizations in pretty much all browsers. [0] outside of the global scope through `window` but all variables are clearly local here so that doesn't apply.
- bsimpson 13y ago
  Doh! Good point.

michaelwww 13y ago

This article taught me how to find a major leak in my Dart2js code so thanks John McCutchan & Loreena Lee http://www.html5rocks.com/en/tutorials/memory/effectivemanagement/ http://www.html5rocks.com/en/tutorials/memory/effectivemanag...

Tloewald 13y ago

It'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.

ufo 13y ago
Not only setInterval though. This sort of leak can also show up if the closure is a DOM event handler or is otherwise exposed.
- Tloewald 13y ago
  It can show up anywhere — and in fact event handlers are a major source of leaks in general — but the point is that the leak doesn't occur as frequently as you might expect not that it occurs.

glasser 13y ago

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

richardofyork 13y ago

First, it is not a bug in JavaScript at all. Here is a technical explanation of why there is a memory leak and how to fix the problem. 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. 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. 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. 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. 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.   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'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. 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. 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.  For a detailed explanation of closures in JavaScript, see "Understand JavaScript Closures With Ease": http://javascriptissexy.com/understand-javascript-closures-with-ease/ http://javascriptissexy.com/understand-javascript-closures-w...

nknighthb 13y ago
You seem to be describing behavior that the article did not. Specifically, the retention of 'str' because of the 'logIt' function. If you read more closely, you'll notice that logIt was not the cause of 'str's retention, but instead, the doSomethingWithStr function was. When logIt is present by itself, str is not retained. Only when doSomethingWithStr is added alongside logIt is str retained.

btilly 13y ago

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't. Careful analysis of this will have many edge cases to worry about. Implementing the simplest thing that is correct according to the spec sounds like a good idea to me.

glasser 13y ago
Yes, but you can statically determine whether or not eval is called. A world-class JavaScript environment like V8 is far past "the simplest thing that is correct according to the spec". In fact, the JavaScript spec doesn't actually define anything about garbage collection! You can create a compliant ECMAScript-262 runtime that literally never collects any garbage ever. That's certainly the simplest correct thing. But it's a bad idea! V8 already goes to the trouble of figuring out whether or not a given variable needs to be stored in the lexical environment or not. Specifically: variables that are not used in ANY closures and where there is no eval in sight can be stored outside of the lexical environment. This is great, and better than many similar programming language environments offer. It just would be even better if they went one step farther.
- btilly 13y ago
  V8 attempts to be fast. There is a trade-off between sophisticated analysis during compilation, and page performance. I'm not as convinced as you that they haven't found the right balance.

glasser 13y ago

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

finnw 13y ago

>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. That is basically how Lua implements closures. https://bugzilla.mozilla.org/show_bug.cgi?id=542074 https://bugzilla.mozilla.org/show_bug.cgi?id=542074

doctorpangloss 13y ago

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.

oakaz 13y ago

The example code looks really silly. As others said, it's just bad programming.

deleted 13y ago

[deleted]

gsg 13y ago

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

glasser 13y ago
Cool! A major reason I made this post was because I assumed that this must be something people had seen before, but I had trouble finding any references to it in the JavaScript context. I'll add a link to http://flint.cs.yale.edu/flint/publications/escc.pdf http://flint.cs.yale.edu/flint/publications/escc.pdf to the blog post!