Adding kill functionallity for win.

[Igmat]

Hotfix for #40.
Used implementation for kill function from node-cross-spawn-with-kill so many thanks to @kentcdodds.

This fix will provide sync killing in win32 environment and will use default node implementation for *nix.

Motivation

Due to this discussion and that kill is part of node API I think that it's not out of scope for this package.

TODO:

• Add tests.

[kentcdodds]

This should probably be in your global gitignore.

[Igmat]

You are right. I'll revert it in next commit.

[Igmat]

I've added tests, but they are useless as for now, because they don't fail when this kill implementation is removed. Seems, that my case here is much more complicated then just not killing child process and it failed only with some special circumstances that I can't figure out right now.

[satazor]

@Igmat the motivation is that, on windows, the kill option offered by NodeJS does not work?

[Igmat]

Partially. Actually in simple cases it works, and that's why those tests do not fail. But in more complicated case default kill works good on *nix systems and doesn't on Windows. As for now, I'm creating test that fully repeats my case and narrow it, to find out minimal circumstances when it will fail with default kill.

[satazor]

@Igmat Yes a reproducible test case would be awesome. I see many popular projects using cross-spawn and never heard of this problem. One of those projects is execa which uses .kill().

[Igmat]

I've added pretty big complicated case and commented new kill implementation in order to show that it fails on Windows without it, and runs successfully on *nix. You are able to uncomment it and see that it runs also successfully on Windows with it. Now, I'll start to remove extra dependencies to narrow it to the most simple case.

[Igmat]

As I said it fails on Windows. See this build. Strange thing that it was succeeded for node v0.12.

[Igmat]

Ok, so as I understand problem is that I spawn jest in watch mode and it spawns something else and it prevents normal killing in Windows systems. So now, I'll try to investigate how actually jest's watch works and find out what happens with processes.

[Igmat]

@satazor, ok I've done my best to test this stuff. But as for now, I have no idea how to narrow it more... I'll probably ask guys from Jest what actually happens when it called with --watch/--watchAll flags and come back to it later.
But here is a test for it, and you can easily check that this fix forces your package to act same on Windows and *nix systems. I guess that it is enough to accept this PR, is it?

[Igmat]

@kentcdodds may be you have some thoughts on it?

[kentcdodds]

If Jest's spawning is causing issues for you, you might try the --runInBand flag and see if that makes a difference.

[johanneswuerbach]

In testem I'm using similar code https://github.com/testem/testem/blob/f32d3edb38852d44fe842f0bceab132ca4ccbe6e/lib/process-ctl.js#L200-L220 as libuv under Windows always force kills processes (no SIGTERM) https://github.com/libuv/libuv/blob/912d42b7285fc29869472106d1c736f34ac91521/src/win/process.c#L1171, which in some cases causes child processes of the spawned process to never be killed.

The magic is the \T flag here.

I would also like to see this merged, but please with support for disabling \F somehow as GUI applications can actually be gracefully terminated under Windows, just terminal apps always require \F.

[satazor]

I'm preparing a new release and I'm looking at this PR again.

Regarding overwriting kill, how do you expect to support the signal to send?

[satazor]

I've landed a major overhaul to the project in the master branch. This means that the changes on this PR need to be rebased.

For now, I will close this PR since there's no answer for my question regarding specifying signals with kill. I might reopen if necessary.

[Igmat]

It was too long ago. Actually, I'm not an expert in that how processes managed in Windows, so I can't answer for your previous question @satazor. I just wanted to have same behavior at different systems for CI purposes and all I need was just simple kill().
Also, it seems that @johanneswuerbach knows much more about different flags and etc. If it still makes sense for you, I may rebase this branch.

[satazor]

Unfortunately, not having a cross platform kill() is a blocker for me. If that it's improved to at least support the Windows signals, we can reopen this MR.