Memory leak when using setTimeout

[mafintosh]

Hey everybody.

I'm having some issues when using setTimeout/setInterval regarding
memory leaks.
It seems as if setTimeout keeps a reference to the function parsed to
it and therefore never releases resources used by the function.

I've tried creating an example that illustrates this problem:

https://gist.github.com/848161

On my machine this never releases any RAM (running v4.1)

If I comment out the setTimeout in the for-loop it garbage collects
all the used buffers

Any thoughts/solutions?

[Tim Caswell]

You inner timeout holds references to `bs` and `c` and `bs` holds a reference to every buffer ever created.  It would be a bug for the system to GC something you have an explicit reference to.  Does it let them go after your function in the timeout is executed?

The reference chain is something like this:

root -> timeoutcallback -> closure -> bs -> buffers

Once the timeout fires, I assume that the callback is removed from the global reference root making the subtree orphaned and eligible for gc.

--
You received this message because you are subscribed to the Google Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com.
To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.

[mafintosh]

On Mar 1, 12:15 am, Tim Caswell <t...@creationix.com> wrote:
> Does it let them go after your function in the timeout is executed?

No it doesnt seem to let them go after the timeout has executed.

[Hsu Ping Feng]

Since bs holds all reference to the newly created Buffers in the for loop, and the inner timeout function refers to bs, all the references kept in bs will not have a change to be GCed until all inner timeout functions executed.

2011/3/1 mafintosh <mathi...@gmail.com>

On Mar 1, 12:15 am, Tim Caswell <t...@creationix.com> wrote:
> Does it let them go after your function in the timeout is executed?

No it doesnt seem to let them go after the timeout has executed.

--

You received this message because you are subscribed to the Google Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com.
To unsubscribe from this group, send email to nodejs+un...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/nodejs?hl=en.

--
AUFKLÄRUNG ist der Ausgang des Menschen aus seiner selbstverschuldeten Unmündigkeit. Unmündigkeit ist das Unvermögen, sich seines Verstandes ohne Leitung eines anderen zu bedienen. Selbstverschuldet ist diese Unmündigkeit, wenn die Ursache derselben nicht am Mangel des Verstandes, sondern der Entschließung und des Mutes liegt, sich seiner ohne Leitung eines andern zu bedienen. Sapere aude! Habe Mut, dich deines eigenen Verstandes zu bedienen! ist also der Wahlspruch der Aufklärung.

[Marco Rogers]

fillano is correct.  You're setting 30 inner timeout functions.  Each of those has a reference to bs with the buffers in it.  So until all of those execute, finish and get GCed, bs will hang around.  This is what you should expect.

Also, I'm not sure why you need the "just to keep the process running" timeout at the end.  As long as there is a function in the event loop queue, node will keep running.  Try removing that. It should run the same.

One more minor thing I'm not sure you're aware of, you will not see the bs.length count up as the timeouts fire.  The entire for loop completes first and these things are done before any of the inner functions fire.  So bs.length will always be 30.

I might've missed something so double check my assumptions.

:Marco

[Liam Doherty]

Hello,

It turns out that V8 is able to GC the buffers you create - it just
never tries. You can observe this by adding the --trace-gc parameter
when you run node.

If you add some ongoing allocation, V8 will GC and collect the
unreferenced buffers, e.g. add this to the bottom of your benchmark
script:

setInterval(function(){
var b = new Buffer(1024*1024*100);
}, 500);

Smaller buffers will work too, but take longer to trigger GC.

This makes sense if you think about it - V8 may be assuming that
there's no point GCing when the total memory usage has remained the
same for awhile - especially since from the point of view of the V8
heap, only a few small objects have become collectible (which just
happen to be attached to big chunks of external memory.) Note that
reducing the delay on your 10 second timeout to e.g. 1 second also
solves the problem. I suspect your memory leak problem in your real
application may be unrelated to this behaviour.

This isn't really related, but it is very important for avoiding bugs:
you should never define an anonymous function inside a loop, since the
behaviour (as you can see with your gist) is unintuitive. Effectively
all variables (and anonymous functions) defined in a loop in
Javascript behave in the same way as if they were defined outside the
loop. I prefer to use the underscore.js library's iteration functions
instead for this reason.

Cheers,
Liam

On Mon, Feb 28, 2011 at 2:23 PM, mafintosh <mathi...@gmail.com> wrote:

[Liam]

On Mar 1, 7:03 pm, Liam Doherty <lfdohe...@gmail.com> wrote:
> This isn't really related, but it is very important for avoiding bugs:
> you should never define an anonymous function inside a loop, since the
> behaviour (as you can see with your gist) is unintuitive.  Effectively
> all variables (and anonymous functions) defined in a loop in
> Javascript behave in the same way as if they were defined outside the
> loop.

I sometimes do this for object member iteration:

function ex(iArg) {
for (var a in object) {
async(args, function(err) {
...
delete object[a]
ex(iArg)
})
return
}
}

[Tim Caswell]

If you're ok with the async calls being run in parallel, then it's much easier and simpler to forEach over the keys of the object.  This creates a new closure for each loop iteration and things work as expected.  It's pretty fast too.

 Object.keys(object).forEach(function (key) {

    value = object[key]

    async(args, function (err, result) {

       // Do something using key, value, and the async result

    })

 })

If you want the items to not run in parallel, then recursion is needed, but please don't use for..in unless you really intend and need to get all the enumerable keys of the object and all it's prototypes.  If you want to save the cost of the closure from .forEach and don't need a scope for each loop iteration, then just do a normal for(;;) loop over the Object.keys.  Just be aware of var hoisting.

--

[mafintosh]

This makes sense! Thanks :)