Improve ChildProcess::killed property behaviour. Or update documentation.

[eduardbme]

• Subsystem: doc and child_process

According to documentation (https://nodejs.org/dist/latest-v9.x/docs/api/child_process.html#child_process_subprocess_killed) the killed property has invalid semantic in comparison to ChildProcess::kill method behaviour (internal/child_process.js)

Concretely, the problematic code is

var err = this._handle.kill(signal);
if (err === 0) {
  /* Success. */
  this.killed = true;
  return true;
}

After the this._handle.kill(signal); method call we have no idea whether the 3-d party process was killed, or it just received a signal, processed it and continues working (it maybe be possible even with such 'kill' signals like SIGINT and SIGTERM).

So I propose to discuss what can be done in this direction to improve expected behaviour.

The easiest solution (and maybe the right one) is to move this.killed = true; line into _handle.onexit function and fix to the following code:

if (signalCode) {
  this.signalCode = signalCode;
  if (this._killWasCalled) { (1)
    this.killed = true;
  }
} else {
  this.exitCode = exitCode;
}

The if block (1) is needed cuz now documentation says '...indicates whether the child process was successfully terminated using subprocess.kill()'. We can eliminate this block, but we will need to update documentation also.

Strictly speaking, the killed field isn't very helpful in terms of identifying when process will exit.
We usually use exitevent for that. But, nevertheless, it's strange to see childProcess.killed === true, when in reality process works fine.

[cjihrig]

My take on this is that it is a documentation issue: #16748

[eduardbme]

@cjihrig

why not update code ?

kill/killed is historically bad name for just signal notification, but we do not have to follow this rule.

Moreover, the killed param that indicates just correct signal notification isn't very helpful cuz the kill method already returns true for that purpose. Logic duplication and no advantage to use killed param at all.

[cjihrig]

why not update code ?

Updating the code is much more likely to break existing applications in non-obvious ways. The 'exit' event also provides the signal used to terminate the process, so you can check that.

kill/killed is historically bad name for just signal notification, but we do not have to follow this rule.

I'm not saying it's a great name, but this flag is literally indicating the result of calling kill(2) (on non-Windows platforms).

[eduardbme]

What if we will deprecate this property in the next versions ?

I do not see any advantage to use it in that form. That's rudimental functionality.