kill does not work anymore with latest releases (0.7.5, 0.7.6)

[jupe]

Environment details

• OS: linux/eg rasbian
• OS version: all I think
• node-pty version: 0.7.5, 0.7.6

Issue description

ptyProcess.kill() does not have affect anymore.

Reproducing easily by spawning sleep 10 :

var pty = require('node-pty');
var ptyProcess = pty.spawn('sleep', [10], {});
ptyProcess.on('exit', (exitcode, signal) => {
  console.log(exitcode, signal);
});
ptyProcess.kill('SIGKILL')

Above script kill directly when using node-pty@v0.7.4, but latest releases waits until sleep is done unexpectedly.

[jupe]

Probably this PR causes issue: #169
Especially this line: https://github.com/Microsoft/node-pty/pull/169/files#diff-0e9d5f27e04ca315ef8d403ff86bf5dfR241 . If I hack that part a bit so that process.kill is called every time above script seems to work again.

[Tyriar]

Thoughts @Daniel-Abrecht @jerch?

[Daniel-Abrecht]

After analyzing the problem further, it turned out that the ioctl returned an EINVAL error. After looking at the linux kernel sources, I figured out that linux only allows this ioctl for SIGINT, SIGQUIT and SIGTSTP: https://github.com/torvalds/linux/blob/master/drivers/tty/pty.c#L211

I've only ever tried sending SIGINT or SIGQUIT, so I never noticed this problem before. I'll start working on a patch to check if pty.kill throws an error and call process.kill in that case tomorrow evening, it's already really late in my timezone. Sorry for the inconvenience.

[jupe]

Thank you Daniel for investigating this. Let me know as soon as patch is available so I could try out it also.

[jerch]

Ah yes, linux support for the pty process group signal is still pretty new, seems they did not implement it for all signal types. Since the doc for this ioctl is not existing at all it is hard to say why they went with those signals and left the others out.

Regarding the problem at hand - should we add an optional argument to kill to indicate whether to target the single process or the process group?

[Tyriar]

@jerch like this?

kill(signal?: string, sendToProcessGroup?: boolean): void;

Seems reasonable.

[Daniel-Abrecht]

I've created a pull request. Please test if everything works again. Someone also needs to try if the windows version still compiles, since I don't have a windows pc to try that.

[jupe]

Fix seems to help, thanks @Daniel-Abrecht. @Tyriar can we get this in patch release soon ?

[jupe]

Any news when we get release ?

[Tyriar]

@jupe chatting with legal about CLA issues, I recommend sticking with 0.7.4 in the meantime.

[Tyriar]

I reverted the offending change 7360eb7