[tolmasky]

> 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]

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]

Hmmm, idiomatic Node.js code creates closures everywhere. (And you may be right that this is just asking for memory leaks.)

[crazygringo]

Let me just clarify -- creating large closures to wrap libraries in -- that kind of thing is good. The (function(){...})(); pattern is excellent for not polluting the global variable namespace. And single library objects are great too -- they're generally only created once, so there's no memory leaks.

And closures for things like callbacks for AJAX etc. -- these are generally fine, because once the AJAX returns and the callback function finishes executing, then once the AJAX object gets garbage-collected, the callback will too, and its closure.

But if you're leaving "infinite-lived" references around to functions within closures, repeatedly, whether it's via an infinitely long setInterval, or whether you're just storing function references in some array you build up and never remove from -- that's just bad programming. Of course it's a "memory leak", but you, the programmer, intentionally asked for it. It's not the language's fault -- that's how the language is supposed to work.

[jcheng]

> And closures for things like callbacks for AJAX etc. -- these are generally fine, because once the AJAX returns and the callback function finishes executing, then once the AJAX object gets garbage-collected, the callback will too, and its closure.

I'm not sure that this is as fine as you think it is. Those callbacks may invoke other callbacks, which may access any variable that's in its lexical scope, and as the Meteor bug shows, you may be inadvertently holding on to not the callback itself, but the environment it lives in.

I think this may be exacerbated in promise-oriented Node.js code since it is so easy to take a promise and stash it in a list or object (which we do in my current project[0], and indeed this code has been the source of memory leaks in the past and may still be leaking).

[0] https://github.com/rstudio/shiny-server/blob/master/lib/sche...

[ulisesrmzroche]

Yeah, you're right. They're not getting that closures can outlive the function that created it. They literally wrote an infinite loop here.

[euroclydon]

I, for one, am not enamored with JavaScript's cuteness. I'm all prototype all the way -- boring stuff that the closure compiler can understand.