Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix execFile timeouts, improve tests

It seems that a parent will not get a SIGCHLD if the child is killed by the
parent? It's unclear, so make 'exit' callback manually.
  • Loading branch information...
commit 6570cd99e51adb082d0e0076a1205cd867f4ce41 1 parent 5a98fa4
@ry ry authored
Showing with 56 additions and 19 deletions.
  1. +25 −14 lib/child_process.js
  2. +31 −5 test/simple/test-exec.js
View
39 lib/child_process.js
@@ -61,10 +61,31 @@ exports.execFile = function (file /* args, options, callback */) {
var stdout = "";
var stderr = "";
var killed = false;
+ var exited = false;
+
+ function exithandler (code, signal) {
+ if (timeoutId) clearTimeout(timeoutId);
+ if (exited) return;
+ exited = true;
+ if (!callback) return;
+
+ if (code === 0 && signal === null) {
+ callback(null, stdout, stderr);
+ } else {
+ var e = new Error("Command failed: " + stderr);
+ e.killed = killed;
+ e.code = code;
+ e.signal = signal;
+ callback(e, stdout, stderr);
+ }
+ }
function kill () {
- child.kill(options.killSignal);
+ var c = child.kill(options.killSignal);

Just curious, why store the return val?

@ry
ry added a note

mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
killed = true;
+ process.nextTick(function () {
+ exithandler(null, options.killSignal)
+ });
}
var timeoutId;
@@ -92,19 +113,9 @@ exports.execFile = function (file /* args, options, callback */) {
}
});
- child.addListener("exit", function (code, signal) {
- if (timeoutId) clearTimeout(timeoutId);
- if (!callback) return;
- if (code === 0 && signal === null) {
- callback(null, stdout, stderr);
- } else {
- var e = new Error("Command failed: " + stderr);
- e.killed = killed;
- e.code = code;
- e.signal = signal;
- callback(e, stdout, stderr);
- }
- });
+ var pid = child.pid;

why are we grabbing the pid here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ child.addListener("exit", exithandler);
return child;
};
View
36 test/simple/test-exec.js
@@ -35,22 +35,48 @@ exec("ls /DOES_NOT_EXIST", function (err, stdout, stderr) {
}
});
+
+
+var sleeperStart = new Date();
exec("sleep 3", { timeout: 50 }, function (err, stdout, stderr) {
+ var diff = (new Date()) - sleeperStart;
+ console.log("sleep 3 with timeout 50 took %d ms", diff);
+ assert.ok(diff < 500);
assert.ok(err);
assert.ok(err.killed);
assert.equal(err.signal, 'SIGKILL');
});
-var killMeTwice = exec("sleep 3", { timeout: 1000 }, function killMeTwiceCallback(err, stdout, stderr) {
- assert.ok(err);
- assert.ok(err.killed);
- assert.equal(err.signal, 'SIGTERM');
-});
+
+
+var startSleep3 = new Date();
+var killMeTwice = exec("sleep 3", { timeout: 1000 }, killMeTwiceCallback);
+
process.nextTick(function(){
+ console.log("kill pid %d", killMeTwice.pid);
+ // make sure there is no race condition in starting the process
+ // the PID SHOULD exist directly following the exec() call.
+ assert.equal('number', typeof killMeTwice._internal.pid);
+ // Kill the process
killMeTwice.kill();
});
+function killMeTwiceCallback(err, stdout, stderr) {
+ var diff = (new Date()) - startSleep3;
+ // We should have already killed this process. Assert that
+ // the timeout still works and that we are getting the proper callback
+ // parameters.
+ assert.ok(err);
+ assert.ok(err.killed);
+ assert.equal(err.signal, 'SIGKILL');
+
+ // the timeout should still be in effect
+ console.log("'sleep 3' was already killed. Took %d ms", diff);
+ assert.ok(diff < 1500);
+}
+
+
exec('python -c "print 200000*\'C\'"', { maxBuffer: 1000 }, function (err, stdout, stderr) {
assert.ok(err);
@aheckmann

why are we grabbing the pid here?

@aheckmann

Just curious, why store the return val?

Please sign in to comment.
Something went wrong with that request. Please try again.