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

Not allowing string was a change from v0.10 behaviour, commented on in
joyent/node#7991. Allow them again, but still check that argument is
numberish. Also, simplify the fragile and non-portable test code
introduced in 832ec1cd507 that required fixups 2a415358ee, and
ef3c4ed3d.

PR-URL: https://github.com/joyent/node/pull/8531
Reviewed-by: Trevor Norris <trev.norris@gmail.com>

src/node.js

@@ -641,8 +641,8 @@
     process.kill = function(pid, sig) {
       var err;
 
-      if (typeof pid !== 'number' || !isFinite(pid)) {
+      if (pid != (pid | 0)) {
-        throw new TypeError('pid must be a number');
+        throw new TypeError('invalid pid');
       }
 
       // preserve null signal

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 */
+// Test kill argument processing in valid cases.
-if (process.platform === 'win32') {
+//
-  pass = true;
+// Monkey patch _kill so that we don't actually send any signals, particularly
-  clearInterval(wait);
+// that we don't kill our process group, or try to actually send ANY signals on
-  return;
+// windows, which doesn't support them.
-}
+function kill(tryPid, trySig, expectPid, expectSig) {
-
+  var getPid;
-process.once('SIGHUP', function() {
+  var getSig;
-  process.once('SIGHUP', function() {
+  var origKill = process._kill;
-    pass = true;
+  process._kill = function(pid, sig) {
-    clearInterval(wait);
+    getPid = pid;
-  });
+    getSig = sig;
-  process.kill(process.pid, 'SIGHUP');
-});
-
-
-/*
- * 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);
-
-    // make sure that _kill is passed the correct args types
-    assert(typeof pid === 'number');
-    assert(typeof sig === 'number');
 
     // un-monkey patch process._kill
     process._kill = origKill;
-    process._kill(process.pid, sig);
+  };
+
+  process.kill(tryPid, trySig);
+
+  assert.equal(getPid, expectPid);
+  assert.equal(getSig, expectSig);
 }
 
-process.kill(0, 'SIGHUP');
+// 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).
+
+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);
+
+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);