Node: `childProcess.killed` should be `true` after `process.kill(childProcess.pid)` is called

[ehmicky]

Is your feature request related to a problem? Please describe.
childProcess.killed is inconsistent depending on how the child process was killed:

const { spawn } = require('child_process')
const childProcess = spawn('sleep', [5e6])
childProcess.kill()
console.log(childProcess.killed) // true

const { spawn } = require('child_process')
const childProcess = spawn('sleep', [5e6])
process.kill(childProcess.pid)
console.log(childProcess.killed) // false

Describe the solution you'd like
killed should be true when the process is killed through an external process (process.kill() or kill in the terminal).

Versions

Node 12.1.0, Ubuntu 19.04.

[bnoordhuis]

Your suggested change is problematic because of the documented behavior:

The subprocess.killed property indicates whether the child process successfully received a signal from subprocess.kill(). The killed property does not indicate that the child process has been terminated.

.killed has always worked this way (it goes back to from before Node.js even ran on Windows) so changing it now might be risky.

[ehmicky]

Thanks for your answer @bnoordhuis.

Could this either:

• be a breaking change (next major release), if the new behavior makes more sense?
• be done using a different property, to maintain backward compatibility?

[bnoordhuis]

Either option works but both have their share of issues:

• Estimating the fallout from a backwards incompatible change is hard
• Having two similar-but-slightly-different properties is confusing

My preference would be option 1 if we could be reasonably sure that the fallout is tractable, otherwise option 2 (or option 3: do nothing.)

[sindresorhus]

Would probably be useful to try to do the change (1) and then run it through https://github.com/nodejs/citgm

[sam-github]

These things are really, really different:

childProcess.killed should be true after process.kill(childProcess.pid) is called

and

killed should be true when the process is killed through an external process (process.kill() or kill in the terminal).

I don't think .killed is intended to mean what you want, and the information you most likely want is already available. Specficially, I think the .signalCode property has the meaning you are looking for.

Right now, .killed means child_process.kill() was called.

It does _not_ mean that the child_process terminated, it could be alive and well:

const { spawn } = require('child_process')
const childProcess = spawn('sleep', [5e6])
childProcess.kill(0)
console.log(childProcess.killed) // true
childProcess.on('exit', (code, signal) => {
  console.log('terminated:', signal||code); // ... wait for re6 seconds... I didn't, but it should print 
             // terminated: 0
             // eventually....
});

When a child process does terminate, for _any_ reason, /bin/kill, shell "kill" builtin, process.kill, etc, etc., then the 'exit' event occurs, and gives the reason for its termination (which could be a signal if it terminated due to a signal):

const { spawn } = require('child_process')
const childProcess = spawn('sleep', [5e6])
require('child_process').execSync(`kill -TERM ${childProcess.pid}`)
console.log(childProcess.killed) // false
childProcess.on('exit', (code, signal) => {
  console.log('terminated:', signal||code); // terminated: SIGTERM
});

Also, when the exit event is emitted, the .exitCode and .signalCode are set.

It is theoretically possible in process.kill() to search all existing ChildProcess objects, and set their .killed property, but it is definitely _not_ possible to know if a signal was sent by some thirdparty/external process. At least, its not possible to know unless the process allows that signal to terminate it (it doesn't have to). At which time, .signalCode will be set, as it is now.

[ehmicky]

Hum this makes sense, thanks for explaining the rational behind this.

From your explanation, I understand that childProcess.killed actually means "a signal (including non-termination signals) was sent to the child process from the current Node.js process"?

I am wondering whether this might cause some confusion for some users. I am also wondering which practical use case this property can have that is not fulfilled with the .exitCode and .signalCode properties or the exit event?

[sam-github]

childProcess.killed actually means "a signal (including non-termination signals) was sent to the child process from the current Node.js process"?

Not quite. If it was as you say, process.kill() would set it, but it does not, as you noticed in the examples.

Its simpler than that: childProcess.killed means that childProcess.kill() was called (on that specific childProcess). This is, btw, exactly what the docs say: https://nodejs.org/api/child_process.html#child_process_subprocess_killed

Its useful for user-code to guard against calling .kill() multiple times on the same childProcess object if for some reason the user-code wants to avoid that. It might even be used internally for that purpose (I did not grep the source to find out). Users could always track the killed state themselves, but its reasonably common in the Node.js API to track this kind of state.

[ehmicky]

This makes sense. Thanks for clarifying this @sam-github!