This repository has been archived by the owner. It is now read-only.

Memory leak when running setInterval in a new context #1007

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

Comments

Projects
None yet
3 participants
@GeorgeBailey

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)

@konobi

This comment has been minimized.

Show comment
Hide comment
@konobi

konobi May 4, 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.

konobi commented May 4, 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.

@GeorgeBailey

This comment has been minimized.

Show comment
Hide comment
@GeorgeBailey

GeorgeBailey May 4, 2011

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

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

@konobi

This comment has been minimized.

Show comment
Hide comment
@konobi

konobi May 4, 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.

konobi commented May 4, 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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 4, 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).

ghost commented May 4, 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).

@GeorgeBailey GeorgeBailey reopened this May 4, 2011

@GeorgeBailey

This comment has been minimized.

Show comment
Hide comment
@GeorgeBailey

GeorgeBailey May 4, 2011

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.

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.

@d-ash

This comment has been minimized.

Show comment
Hide comment
@d-ash

d-ash 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

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

@konobi

This comment has been minimized.

Show comment
Hide comment
@konobi

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

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.

@konobi konobi closed this May 23, 2011

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.