Skip to content

Commit

Permalink
fix(script): memory leak and performance improvement
Browse files Browse the repository at this point in the history
There is a bug in node with vm.runInNewContext: nodejs/node-v0.x-archive#6552
This was used for every script call by deployd, and for example GET scripts are being run for every record returned -> so if you need to return 1000 records, the scripts would run 1000 times resulting in big memory leaks.

With this change, even 15000 executions of a script take now just 300 ms on my machine and use virtually no extra memory, compared to huge memory leaks and over several minutes with the old code.
  • Loading branch information
andreialecu committed Jan 27, 2015
1 parent 665e091 commit eb54189
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 2 deletions.
26 changes: 24 additions & 2 deletions lib/script.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,34 @@ var vm = require('vm')

function Script(src, path) {
try {
this.compiled = vm.createScript('(function() {' + src + '\n}).call(_this)', path);
this.src = src;
this.path = path;
} catch(ex) {
this.error = ex;
}
}

Script.prototype.runInSandbox = function (sandbox) {
var functionArgs = Object.keys(sandbox);

// remove the argument 'this' from our list of passed argument, because it is a reserved word
functionArgs.splice(functionArgs.indexOf('this'), 1);

var script = '(function (' + functionArgs.join(',') + ') { return (function() {' + this.src + '\n}).call(_this); })';
var func = vm.runInThisContext(script, this.path);

// pass our arguments from the sandbox to the function
var args = [];
functionArgs.forEach(function (p) {
if (p === '_this' && !sandbox[p]) {
args.push({});
} else {
args.push(sandbox[p]);
}
});
return func.apply(null, args);
};

/**
* Run the current script in the given sandbox. An optional domain may be provided to extend the sandbox exposed to the script.
*/
Expand Down Expand Up @@ -112,7 +134,7 @@ Script.prototype.run = function (ctx, domain, fn) {
var err;

try {
this.compiled.runInNewContext(scriptContext);
this.runInSandbox(scriptContext);
} catch(e) {
err = wrapError(e);
scriptContext._error = err;
Expand Down
14 changes: 14 additions & 0 deletions test/script.unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ describe('script', function(){
var s = new Script('if(!foo) throw "foo not passed"');
s.run({}, {foo: 123}, done);
});

it('should not be slow and leak memory', function (done) {
var s = new Script('if(!foo) throw "foo not passed"');
var time = Date.now();
var numDone = 0;
for (var i = 0; i < 15000; i++) {
s.run({ }, { foo: 123 }, function() {
numDone++;
if (numDone >= 15000) {
done();
}
});
}
});
});

describe('async', function(){
Expand Down

0 comments on commit eb54189

Please sign in to comment.