Make `.killed === true` regardless of how it was killed about execa

[sindresorhus]

I also got confused between those two properties while checking this issue. Should we rename execaError.killed to avoid that confusion? For example execaError.aborted, execaError.terminated, or anything else?

Yes. I prefer using is or was prefix for the property.

[SamVerschueren]

I was looking into implementing this but I need some help first. In the spawned.on('exit') handler I wanted to set spawned.killed to true if the signal indicates that it was killed. But what signals indicate a kill? Or can we check it in a different way?

I was now checking for SIGTERM and SIGINT but don't think that's enough.

[okonet]

@SamVerschueren I think https://github.com/Tapppi/async-exit-hook/blob/master/index.js#L89-L94 is a good enough list

[ehmicky]

I've opened an issue with Node.js to make childProcess.killed true even when the child process was killed through process.kill() or kill in the terminal (as opposed to childProcess.kill()).

[bnoordhuis]

But what signals indicate a kill?

If signal != null in proc.on('exit', (code, signal) => { /* ... */ }), then that's the signal that terminated the process.

If your question is "how do I determine whether it's sent by kill(2)", the answer is "you don't." :-)

[ehmicky]

Yes I think @bnoordhuis fix for it (checking signal on exit) is the correct one. However I still think that providing this as part of Node.js might be a good idea (as opposed to execa).

[ehmicky]

From the discussion at nodejs/node#27490, it appears childProcess.killed just means "childProcess.kill() was called". Regardless of whether it terminated the process. For example childProcess.kill(0) sets childProcess.killed as true. The purpose of childProcess.killed seems to be mostly about idempotency, e.g. preventing calling childProcess.kill() multiple times.

Instead, that discussion highlights that the best way to check this is to look at the signal that is passed as a second argument to the exit event of the child process.

In other words, we might want to replace:

execa/lib/error.js

Line 84 in fcf648e

error.killed = killed && !timedOut;

By:

error.killed = error.signal !== undefined;

The documentation says:

signal The signal by which the child process was terminated.

So, we can assume that if a signal is passed, it was the one that terminated the child process.

What do you think?

[sindresorhus]

👍

We should document that it differs from the one in Node.js.

[ehmicky]

There is actually a difference between:

• childProcess.killed: left as is by Execa, i.e. has the same behavior as core Node.js. It only checks whether childProcess.kill() has been called (even .kill(0)), to prevent from calling it several times.
• execaError.killed: intended to check whether the child process was "killed", i.e. terminated by a signal. At the moment, it does not behave as such. Instead it behaves like childProcess.killed, which can probably be considered a bug.

I.e. since execaError.killed is an Execa addition, we do not differ from Node.js behavior.

I also got confused between those two properties while checking this issue. Should we rename execaError.killed to avoid that confusion? For example execaError.aborted, execaError.terminated, or anything else?

[ehmicky]

Sounds good. Done at #625.