[crazygringo]

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]

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

[Dylan16807]

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

In the logical sense each function has its own closure. There is no way functions should affect which[edit for clarity] variables are closed on by each other. So you shouldn't expect this.

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

In the logical sense functions close over their variables, not all variables that happen to be in scope. So that optimization is needed.

[crazygringo]

If I'm understanding what you're saying, that's just not correct. Try and run this code:

    var a = function() {
      var z = "a";
      window.b = function() { z = "b"; }
      window.c = function() { return z; }
    };
    a();
    window.c(); // returns "a"
    window.b();
    window.c(); // returns "b"

As you can clearly see, the single closure containing the variable "z" is shared between functions b() and c().

So each function does not have its own closure -- closures work "upwards", referencing everything in every scope above them.

[Dylan16807]

You have two closures that share a variable. Think about nesting it deeper.

  var a = function() {
    var z = "foo";
    window.getz = function() { return z; };
    
    var b = function() {
      var y = "bar";
      window.n = function() { z = y; };
    };
    
    var c = function() {
      var y = "baz";
      window.m = function() { z = y; };
    };
    b();
    c();
  };
  a();

Now function window.n and window.m have completely different closures that happen to share the upvalue 'z', but do not share the upvalue 'y'.

[crazygringo]

I don't see how that's refuting my point -- of course they don't share "y" because they're two completely different y's in two completely different local scopes.

I had understood your comment to mean that closures don't share variables, and I was just explaining that they certainly can. Maybe I'd misinterpreted what you were trying to say...

[Dylan16807]

I wasn't trying to say that at all.

I was saying that two functions side by side have distinct closures, no matter how many variables they share.

The reason I said that was to point out that variables closed over by one function shouldn't affect which variables are closed over by nearby functions.

Actually I think I see how to make the wording clearer in the original. Brb.

[Someone]

My $0.02 regarding terminology that may help this discussion: what you call a scope is, in Lisp terms, a (lexical) environment: something that, given a name, can give you a value or a "I don't know an object with that name" error, and that you can give a (name,value) pair (a binding) to create (languages vary a bit in the way they handle the case where the name already is known)

In many languages, environments are nested. For example, in the example:

  var a = function() {
    var y = "bar";
    var z = "foo";
    window.getz = function() { return z; };
    
    var b = function() {
      window.bb = function() { z = y; };
    };
    
    var c = function() {
      window.cc = function() { z = y; };
    };

    var d = function() {
      var q = 42;
      window.dd = function() { z = q; };
    };

    b();
    c();
    d();
  };
  a()

Calling b, c, and d creates three environments. The environment created by calling d adds a binding of q to 42 to its environment; neither the one created by calling b nor the one created by calling c add bindings of its own, but conceptually, these two have their own environment. All three build on the environment of function a; that environment contains bindings for y and z

Normally, when leaving a block, the environment created when entering it can be discarded. Here, however, the environments still can be reached after execution of the block has ended; each of those environments becomes (part of) its own closure when the function returns. The closures created by calling c and by calling d are functionally identical because they both build on the same 'parent' environment and have no bindings of their own, but conceptually, each has its own closure.

More, better info at http://c2.com/cgi/wiki?LexicalClosure and, of course, in SICP, section 3.2 "The Environment Model of Evaluation" (although I remember seeing clearer pictures for that that involve making closures. Maybe they are in the videos?)

[jcampbell1]

> In the logical sense each function has its own closure. There is no way functions should affect the variables closed on by each other. So you shouldn't expect this.

What? The whole point of a closure is that it is shared.

[Dylan16807]

The point of closures is to share variables, but each closure can participate in different shares.

[bmm6o]

Part of the reason people are misunderstanding you is that terminology got abused a little bit upthread.

"I just always assumed that all the variables present in a closure are maintained" should really be (something like) "I just always assumed that all the variables present in a scope that produced a closure are maintained". You're right that a closure only needs to contain the variables that are closed over, and that some of those variables are scoped such that they can appear in multiple closures.

[DanWaterworth]

> You're right that a closure only needs to contain the variables that are closed over.

You would think so, but this assumption fails in the presence of eval.

[bmm6o]

Sure, but that's the only exception, right? If there's no eval call in the function that requires the closure it's easily determined what variables don't need to be included, right?

[ulisesrmzroche]

In JS, functions can refer to variables defined in outer scopes, and can refer to variables defined in outer functions even after those functions have returned. JS functions are first-class objects.

They also store any variables they may refer to that are defined in their enclosing scopes, including the parameters and variables of outer functions.

[bzbarsky]

It's a common enough optimization for JS engines in some form.

In particular, if the closure only reads the variable and nothing can write it, it's way cheaper in terms of performance to just create a separate lexical environment for the closure that contains the things it reads so that you don't have to keep walking up the scope chain on bareword lookups in the closure.

The fact that things then end up not referenced and can be gced is just a side-effect of the primary goal of the optimization: faster access to variables in a closure.

[lesslaw]

Works as per spec, won't fix. Closing.

The definition of "memory leak" is smeared all across the road.