Skip to content
This repository

Memory leak when running setInterval in a new context #1007

Closed
GeorgeBailey opened this Issue May 03, 2011 · 7 comments

4 participants

George Bailey Scott McWhirter Herbert Vojčík Andrey Shubin
George Bailey

Tested in 0.4.7 and there is a leak

var
Context  = process.binding('evals').Context,
Script   = process.binding('evals').Script,
total    = 5000,
result   = null;

process.nextTick(function memory() {
  var mem = process.memoryUsage();
  console.log('rss:', Math.round(((mem.rss/1024)/1024)) + "MB");
  setTimeout(memory, 100);
});

console.log("STARTING");
process.nextTick(function run() {
  var context = new Context();

  context.setInterval = setInterval;

  Script.runInContext('setInterval(function() {}, 0);',
                      context, 'test.js');
  total--;
  if (total) {
    process.nextTick(run);
  } else {
    console.log("COMPLETE");
  }
});

See comments starting with tmpvar: "I am confident this isolates the leak" tmpvar/jsdom#154 (comment)

Also occurs in 0.4.0
tmpvar/jsdom#154 (comment)

Scott McWhirter
konobi commented May 04, 2011

Why are you using the private API for Script/Context? All this functionality has been moved to "vm".

Also note that you are using a class variable to run the script... so every time you run runInContext, a brand new script is being created and kept around at the global level. You should be creating a script object and reusing it.

George Bailey

@konobi: I think the reason for having a new script each time is to isolate the JavaScript that runs in a web page parsed by jsdom. I am only running a jquery file that I control so I guess this is not necessary, but that is the way the code is written.

I am just trying to get this resolved. If the tmpvar's jsdom code can be changed to avoid the memory leak, then I am happy. I don't know why, in fact I was not able to find documentation to describe process.binding('evals').Script.runInContext so I don't know.

Can the script be destroyed to free the memory? Is it easy to rewrite to use vm?

If this is clearly wrong coding, you could describe why on the jsdom bug I linked to, but tmpvar seems to think it is a Node.JS bug. But don't make him re-write without a promise of resolving the memory leak.

Scott McWhirter
konobi commented May 04, 2011

This is closer to what one might expect. Oddly enough, it's still doing something a bit odd.

var
vm       = require("vm"),
total    = 5000,
result   = null;

process.nextTick(function memory() {
  var mem = process.memoryUsage();
  console.log('rss:', Math.round(((mem.rss/1024)/1024)) + "MB");
  setTimeout(memory, 100);
});

console.log("STARTING");
process.nextTick(function run() {
  var script = vm.createScript('setInterval(function() {}, 0);', 'test.js');
  var sandbox = { setInterval: setInterval };
  script.runInNewContext(sandbox);

  total--;
  if (total) {
    process.nextTick(run);
  } else {
    console.log("COMPLETE");
  }
});

Either way you should be using the vm module, that's the stable API... Script and Context may just disappear at some point anyways.

@ry testing this locally did show memory consumption increase, not sure as to the reason.

Herbert Vojčík
herby commented May 04, 2011

@konobi: Script.runInContext(...) does not create a new script globally, it compiles and runs the code ad-hoc (it's the same as public, but undocumented API vm.runInContext).

George Bailey GeorgeBailey closed this May 04, 2011
George Bailey GeorgeBailey reopened this May 04, 2011
George Bailey

Regardless, I would like know how to eliminate the memory increase so I don't have to restart the process every so often. Whether bug fix or workaround I don't really care. That is why I asked if the script or "sandbox" could be explicitly destroyed.

Andrey Shubin
d-ash commented May 22, 2011

That's not a bug.
setInterval() protects function handle from garbage collection (btw, setTimeout() do the same, but it eventually will be timed out). You should clear all timers set by setInterval() and memory will be freed.

Here is the code we use: https://gist.github.com/985918/c2e0284120244de1cefbffe2e984edec746e41db

Scott McWhirter
konobi commented May 23, 2011

@d-ash is correct:


var
vm       = require("vm"),
total    = 5000,
result   = null;

process.nextTick(function memory() {
  var mem = process.memoryUsage();
  console.log('rss:', Math.round(((mem.rss/1024)/1024)) + "MB");
  setTimeout(memory, 100);
});

var script = vm.createScript('setInterval(function() {}, 0);', 'test.js');

console.log("STARTING");
process.nextTick(function run() {
  var intervals = [];
  var _setint = function(){
      var foo = setInterval(arguments);
      intervals.push(foo);
      return foo;
  }
  var sandbox = { setInterval: _setint };
  script.runInNewContext(sandbox);
  delete intervals;

  total--;
  if (total) {
    process.nextTick(run);
  } else {
    console.log("COMPLETE");
  }
});

This shows that memory gets collected properly. The problem is that the context of the setInterval function is global and not the context for the sandbox, so it won't get collected correctly.

Scott McWhirter konobi closed this May 23, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.