add timeout support for kill()

[neekey]

What

Add timeout for kill() function. Now by default the timeout will be 30s, if the killing is not successful after 30s, the callback will be invoked with an timeout error, you can also manually set a timeout value (as seconds):

ps.kill(id, { timeout: 10 }, function(){}); // set the timeout to 10s

[trotzig]

Looks good from my end. I left some suggestions for improvements, but overall it looks like this should work. Thanks for taking the time to help us out!

[trotzig]

It's a little annoying for the test to have to wait the default 30 seconds. I get why you want to cover this case though. I would have been okay with a 5 or 10 second timeout to be honest.

[trotzig]

Picking a lower number here would make tests faster.

[trotzig]

Using setTimeout here seems a little overkill. You could just remember a timestamp when the killing started, then compare the current timestamp against that inside the recursion. That would remove the need for the two variables to keep track of timing out.

[neekey]

That sounds doable and easier to implement, will try : )

[lencioni]

It probably makes sense to bring in something like sinon so you can manually control the clock instead of waiting. This will make tests faster and less flakey. Look at sinon.useFakeTimers.

[neekey]

@lencioni That's a good suggestion, will try : )

[neekey]

@lencioni @trotzig thanks for reviewing this PR and your suggestions, I will do some optimization based on your suggestions, should be easy.

The problem I am facing now is the test cases randomly fails, I put some logs to Travis-CI, it seems like that the result of the process checking is not stable, for the timeout test cases for kill(), since I have already faked the process.kill, the ps.kill should not work, and the checking should always return 1 which means the process is still alive, but according to the console log I put in Travis, sometimes the result returned as 0 even the process is still alive, and the next check becomes 1 back again.

You can see here https://travis-ci.org/neekey/ps/jobs/217466603#L340, so the randomly false checking result causes the test case failed.

That's just very weird and I have to say it happens before that's why there is a confident check in the checking.

Do you guys have any idea about this? Otherwise, I will keep on debugging on it until I can fix it.

[neekey]

I did more debugging, and found somehow in the Travis machine, sometime's the data event of the spawn process just didn't fire, neither did the error event, but the process just fired the exit process, which cause the checking's result as 0 (not found the target process), very weird... https://travis-ci.org/neekey/ps/jobs/217699731#L1338

This happens only for node 5 and 0.12

[neekey]

I added an extra regression for checkConfident to prevent the unstable check result to interrupt the result 26d1902, have created an issue for this #55, will merge this PR now : )