Forcibly kill every forked subprocess.

[jamestalmage]

Killing the parent process skips execution of end listeners
on remaining forked child processes.

This interferes with nyc if processes are kept running after
test completion.

Refrence: istanbuljs/nyc#52

[sindresorhus]

Why are you using promises here? cp.kill() is a synchronous operation. Just do a [].forEach().

[jamestalmage]

result.kill !== process.kill. It is a method I attached to the result. It returns a Promise that resolves with childProcess.on('exit',.... This is waiting for all the child processes to exit before killing the parent.

result.kill also makes sure process.kill is not being called on any processes that have already exited. Because:

May emit an 'error' event when the signal cannot be delivered. Sending a signal to a child process that has already exited is not an error but may have unforeseen consequences: if the PID (the process ID) has been reassigned to another process, the signal will be delivered to that process instead. What happens next is anyone's guess.

Note that while the function is called kill, the signal delivered to the child process may not actually kill it. kill really just sends a signal to a process.

---- https://nodejs.org/api/child_process.html#child_process_child_kill_signal

process.kill may return synchronously, but we want to wait for children to actually exit. exit events certainly are not processed synchronously.

Proof: https://github.com/jamestalmage/process-kill-is-sync

[jamestalmage]

Specifically,
nyc uses signal-exit, which intercepts the kill signal and allows it to finish up writing files to disk before the process is actually killed.

I updated jamestalmage/process-kill-is-sync to illustrate signal-exit delaying the exit.

[sindresorhus]

Oh, right. Hard to read diffs on mobile...

[sindresorhus]

Lgtm.

@vdemedes ?

[sindresorhus]

@jamestalmage Good find btw. I've been experiencing this problem too.

@SamVerschueren This might be related to our nyc problems in Pageres.

[SamVerschueren]

Nice! 👍

Will have a look back at pageres if this PR is merged (and released :))

[vadimdemedes]

I think it would be better to attach kill() method to a returned Promise, like on() - https://github.com/jamestalmage/ava/blob/jt-kill-forked-processes/lib/fork.js#L66.

That way, we won't have weird stuff in test results.

[jamestalmage]

We don't have a reference to the promise in cli.js#exit(), only the result. Hence the need to attach it to the result.

That way, we won't have weird stuff in test results.

Not sure how this would create "weird stuff" in test results.

[vadimdemedes]

Then we better store references to forks, rather than injecting a function in result object.

Not sure how this would create "weird stuff" in test results.

Function in an object with test results is weird stuff.

[vadimdemedes]

So, basically, this PR could be cleaned up by:

1. Attaching kill() on the returned Promise, not the results object
2. Don't resolve here - https://github.com/jamestalmage/ava/blob/jt-kill-forked-processes/lib/fork.js#L39, but resolve/reject based on the exit code:

ps.on('exit', function (code) {
  // resolve/reject
});

inside https://github.com/jamestalmage/ava/blob/jt-kill-forked-processes/lib/fork.js#L36

[jamestalmage]

@vdemedes
Attaching kill() to the promise is doable.

Your second request is problematic. It forces the process to exit before the results promise is resolved. This is problematic for situations where the tests are done, but something is keeping the process running (i.e. you launched a server and fail to shut it down).

[vadimdemedes]

I don't understand your point. I am just suggesting to resolve/reject based on exit, not results. The forked processes will exit anyway, we are just detecting it a bit differently.

[jamestalmage]

The forked processes will exit anyway

Not always, see istanbuljs/nyc#52.

That was primary motivator of this PR.

[jamestalmage]

Here is the relevant comment: istanbuljs/nyc#52 (comment)

Firebase has a known issue where the process doesn't die until you explicitly call process.exit(). But I don't understand this issue well enough / have the right context to know if that's related to this issue

Firebase won't be the last place people run into this (launching an express instance that you fail to destroy, etc).

This PR waits until all the results are in, and then goes back and forcibly kills any child processes that haven't shut themselves down already.

It's necessary to keep process closure and test result promise resolution as separate things.

[SamVerschueren]

This is probably also an issue in pageres with nyc.

[jamestalmage]

@vdemedes
@4d907aa incorporates the "kill() on promise" request.

Personally, I felt the code read better the way I had it originally, but I will defer to you guys.

As an alternate, what if I defined a non-enumerable kill() property on result?

[vadimdemedes]

I think I will merge this PR and pick it up from then, I still have some tips to clean up the code.

Thank you for your work!