Merge pull request #51 from neekey/bugfix/kill_timeout

add timeout support for `kill()`

lib/index.js

@@ -190,6 +190,7 @@ exports.lookup = function (query, callback) {
  * @param pid
  * @param {Object|String} signal
  * @param {String} signal.signal
+ * @param {number} signal.timeout
  * @param next
  */
 
@@ -200,6 +201,8 @@ exports.kill = function( pid, signal, next ){
     signal = undefined;
   }
 
+  var checkTimeoutSeconds = (signal && signal.timeout) || 30;
+
   if (typeof signal === 'object') {
     signal = signal.signal;
   }
@@ -211,27 +214,37 @@ exports.kill = function( pid, signal, next ){
   }
 
   var checkConfident = 0;
+  var checkTimeoutTimer = null;
+  var checkIsTimeout = false;
 
   function checkKilled(finishCallback) {
-    console.log('check kill success', pid, 'confident', checkConfident);
     exports.lookup({ pid: pid }, function(err, list) {
-      console.log('check kill success result:', (list || []).length);
+      if (checkIsTimeout) return;
+
       if (err) {
-          finishCallback && finishCallback(err);
-        } else if(list.length > 0) {
-          checkKilled(finishCallback);
+        clearTimeout(checkTimeoutTimer);
+        finishCallback && finishCallback(err);
+      } else if(list.length > 0) {
+        checkConfident = (checkConfident - 1) || 0;
+        checkKilled(finishCallback);
+      } else {
+        checkConfident++;
+        if (checkConfident === 5) {
+          clearTimeout(checkTimeoutTimer);
+          finishCallback && finishCallback();
         } else {
-          checkConfident++;
-          if (checkConfident === 5) {
-            finishCallback && finishCallback();
-          } else {
-            checkKilled(finishCallback);
-          }
+          checkKilled(finishCallback);
         }
+      }
     });
   }
 
   next && checkKilled(next);
+
+  checkTimeoutTimer = next && setTimeout(function() {
+    checkIsTimeout = true;
+    next(new Error('Kill process timeout'));
+  }, checkTimeoutSeconds * 1000);
 };
 
 /**

package.json

@@ -22,6 +22,7 @@
   },
   "license": "MIT",
   "devDependencies": {
+    "sinon": "^2.1.0",
     "mocha": "^2.4.5"
   }
 }

readme.md

@@ -106,10 +106,13 @@ ps.kill( '12345', 'SIGKILL', function( err ) {
 });
 ```
 
-To be compatible with prior versions, you can use object as the second parameter:
+you can use object as the second parameter to pass more options:
 
 ```js
-ps.kill( '12345', { signal: 'SIGKILL' }, function(){});
+ps.kill( '12345', { 
+    signal: 'SIGKILL',
+    timeout: 10,  // will set up a ten seconds timeout if the killing is not successful
+}, function(){});
 
 ```
 

test/node_process_for_test.js

@@ -1,7 +1,10 @@
 var now = Date.now();
 console.log('[child] child process start!');
 
+function doSomething() {
+  return null;
+}
+
 setInterval(function () {
-  var interval = Date.now() - now;
-  console.log('[child] Timer finished, time consuming: ' + interval);
+  doSomething();
 }, 50);

test/test.js

@@ -2,7 +2,7 @@ var PS = require('../index');
 var CP = require('child_process');
 var assert = require('assert');
 var Path = require('path');
-var IS_WIN = process.platform === 'win32';
+var Sinon = require('sinon');
 
 var serverPath = Path.resolve(__dirname, './node_process_for_test.js');
 var UpperCaseArg = '--UPPER_CASE';
@@ -20,6 +20,16 @@ function killProcess() {
   }
 }
 
+var processKill = process.kill;
+
+function mockKill() {
+  process.kill = function() {};
+}
+
+function restoreKill() {
+  process.kill = processKill;
+}
+
 describe('test', function () {
   before(function (done) {
     PS.lookup({arguments: 'node_process_for_test'}, function (err, list) {
@@ -97,10 +107,11 @@ describe('test', function () {
     });
   });
 
-  for (var i = 0; i < 20; i++) {
-    describe('#kill() test round: ' + i, function () {
+  describe('#kill()', function () {
 
-      it('kill', function (done) {
+    it('kill', function (done) {
+      PS.lookup({pid: pid}, function (err, list) {
+        assert.equal(list.length, 1);
         PS.kill(pid, function (err) {
           assert.equal(err, null);
           PS.lookup({pid: pid}, function (err, list) {
@@ -109,15 +120,20 @@ describe('test', function () {
           });
         });
       });
+    });
 
-      it('should not throw an exception if the callback is undefined', function (done) {
-        assert.doesNotThrow(function () {
-          PS.kill(pid);
-          setTimeout(done, 400);
+    it('should not throw an exception if the callback is undefined', function (done) {
+      assert.doesNotThrow(function () {
+        PS.kill(pid);
+        PS.kill(pid, function() {
+          done();
         });
       });
+    });
 
-      it('should force kill when opts.signal is SIGKILL', function (done) {
+    it('should force kill when opts.signal is SIGKILL', function (done) {
+      PS.lookup({pid: pid}, function (err, list) {
+        assert.equal(list.length, 1);
         PS.kill(pid, {signal: 'SIGKILL'}, function (err) {
           assert.equal(err, null);
           PS.lookup({pid: pid}, function (err, list) {
@@ -126,8 +142,11 @@ describe('test', function () {
           });
         });
       });
+    });
 
-      it('should throw error when opts.signal is invalid', function (done) {
+    it('should throw error when opts.signal is invalid', function (done) {
+      PS.lookup({pid: pid}, function (err, list) {
+        assert.equal(list.length, 1);
         PS.kill(pid, {signal: 'INVALID'}, function (err) {
           assert.notEqual(err, null);
           PS.kill(pid, function(){
@@ -136,5 +155,45 @@ describe('test', function () {
         });
       });
     });
-  }
+  });
+
+  describe('#kill() timeout: ', function () {
+    it('it should timeout after 30secs by default if the killing is not successful', function(done) {
+      mockKill();
+      var clock = Sinon.useFakeTimers();
+      var killStartDate = Date.now();
+      PS.lookup({pid: pid}, function (err, list) {
+        assert.equal(list.length, 1);
+        PS.kill(pid, function (err) {
+          assert.equal(Date.now() - killStartDate >= 30 * 1000, true);
+          assert.equal(err.message.indexOf('timeout') >= 0, true);
+          restoreKill();
+          PS.kill(pid, function(){
+            clock.restore();
+            done();
+          });
+        });
+        clock.tick(30 * 1000);
+      });
+    });
+
+    it('it should be able to set option to set the timeout', function(done) {
+      mockKill();
+      var clock = Sinon.useFakeTimers();
+      var killStartDate = Date.now();
+      PS.lookup({pid: pid}, function (err, list) {
+        assert.equal(list.length, 1);
+        PS.kill(pid, { timeout: 5 }, function (err) {
+          assert.equal(Date.now() - killStartDate >= 5 * 1000, true);
+          assert.equal(err.message.indexOf('timeout') >= 0, true);
+          restoreKill();
+          PS.kill(pid, function(){
+            clock.restore();
+            done();
+          });
+        });
+        clock.tick(5 * 1000);
+      });
+    });
+  });
 });