process: pid can be a string in process.kill()

sam-github · trevnorris · commit 743a009bad64 · 2014-11-17T12:15:54.000-08:00
Not allowing string was a change from v0.10 behaviour, commented on in nodejs/node-v0.x-archive#7991. Allow them again, but still check that argument is numberish. Also, simplify the fragile and non-portable test code introduced in 832ec1c that required fixups 2a41535, and ef3c4ed. PR-URL: nodejs/node-v0.x-archive#8531 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
diff --git a/src/node.js b/src/node.js
@@ -641,8 +641,8 @@
     process.kill = function(pid, sig) {
       var err;
 
-      if (typeof pid !== 'number' || !isFinite(pid)) {
-        throw new TypeError('pid must be a number');
+      if (pid != (pid | 0)) {
+        throw new TypeError('invalid pid');
       }
 
       // preserve null signal
diff --git a/test/simple/test-process-kill-pid.js b/test/simple/test-process-kill-pid.js
@@ -23,15 +23,6 @@
 var common = require('../common');
 var assert = require('assert');
 
-var pass;
-var wait = setInterval(function(){}, 1000);
-
-process.on('exit', function(code) {
-  if (code === 0) {
-    assert.ok(pass);
-  }
-});
-
 // test variants of pid
 //
 // null: TypeError
@@ -43,58 +34,55 @@ process.on('exit', function(code) {
 //
 // Nan, Infinity, -Infinity: TypeError
 //
-// 0: our group process
+// 0, String(0): our group process
 //
-// process.pid: ourself
+// process.pid, String(process.pid): ourself
 
 assert.throws(function() { process.kill('SIGTERM'); }, TypeError);
-assert.throws(function() { process.kill(String(process.pid)); }, TypeError);
 assert.throws(function() { process.kill(null); }, TypeError);
 assert.throws(function() { process.kill(undefined); }, TypeError);
 assert.throws(function() { process.kill(+'not a number'); }, TypeError);
 assert.throws(function() { process.kill(1/0); }, TypeError);
 assert.throws(function() { process.kill(-1/0); }, TypeError);
 
-/* Sending SIGHUP is not supported on Windows */
-if (process.platform === 'win32') {
-  pass = true;
-  clearInterval(wait);
-  return;
-}
+// Test kill argument processing in valid cases.
+//
+// Monkey patch _kill so that we don't actually send any signals, particularly
+// that we don't kill our process group, or try to actually send ANY signals on
+// windows, which doesn't support them.
+function kill(tryPid, trySig, expectPid, expectSig) {
+  var getPid;
+  var getSig;
+  var origKill = process._kill;
+  process._kill = function(pid, sig) {
+    getPid = pid;
+    getSig = sig;
 
-process.once('SIGHUP', function() {
-  process.once('SIGHUP', function() {
-    pass = true;
-    clearInterval(wait);
-  });
-  process.kill(process.pid, 'SIGHUP');
-});
+    // un-monkey patch process._kill
+    process._kill = origKill;
+  };
 
+  process.kill(tryPid, trySig);
 
-/*
- * Monkey patch _kill so that, in the specific case
- * when we want to test sending a signal to pid 0,
- * we don't actually send the signal to the process group.
- * Otherwise, it could cause a lot of trouble, like terminating
- * the test runner, or any other process that belongs to the
- * same process group as this test.
- */
-var origKill = process._kill;
-process._kill = function(pid, sig) {
-    /*
-     * make sure we patch _kill only when
-     * we want to test sending a signal
-     * to the process group.
-     */
-    assert.strictEqual(pid, 0);
+  assert.equal(getPid, expectPid);
+  assert.equal(getSig, expectSig);
+}
 
-    // make sure that _kill is passed the correct args types
-    assert(typeof pid === 'number');
-    assert(typeof sig === 'number');
+// Note that SIGHUP and SIGTERM map to 1 and 15 respectively, even on Windows
+// (for Windows, libuv maps 1 and 15 to the correct behaviour).
 
-    // un-monkey patch process._kill
-    process._kill = origKill;
-    process._kill(process.pid, sig);
-}
+kill(0, 'SIGHUP', 0, 1);
+kill(0, undefined, 0, 15);
+kill('0', 'SIGHUP', 0, 1);
+kill('0', undefined, 0, 15);
+
+// negative numbers are meaningful on unix
+kill(-1, 'SIGHUP', -1, 1);
+kill(-1, undefined, -1, 15);
+kill('-1', 'SIGHUP', -1, 1);
+kill('-1', undefined, -1, 15);
 
-process.kill(0, 'SIGHUP');
+kill(process.pid, 'SIGHUP', process.pid, 1);
+kill(process.pid, undefined, process.pid, 15);
+kill(String(process.pid), 'SIGHUP', process.pid, 1);
+kill(String(process.pid), undefined, process.pid, 15);

Original file line number	Diff line number	Diff line change
`@@ -641,8 +641,8 @@`
`641`	`641`	`process.kill = function(pid, sig) {`
`642`	`642`	`var err;`
`643`	`643`
`644`		`- if (typeof pid !== 'number' \|\| !isFinite(pid)) {`
`645`		`- throw new TypeError('pid must be a number');`
	`644`	`+ if (pid != (pid \| 0)) {`
	`645`	`+ throw new TypeError('invalid pid');`
`646`	`646`	`}`
`647`	`647`
`648`	`648`	`// preserve null signal`