execa icon indicating copy to clipboard operation
execa copied to clipboard

`killSignal` option has no effect

Open geigerzaehler opened this issue 4 years ago • 6 comments

The killSignal option inherited from child_process.spawn() has no effect. If this this option is specified it should set the default signal for killing the child as per the documentation of child_process.spawn().

For example:

// parent.mjs
import { execa } from "execa"

const child = execa("node", ["child.js"], { killSignal: "SIGINT", stdio: "inherit" })

setTimeout(() => {
  child.kill()
}, 100)
// child.mjs
const onExit = require('signal-exit')

onExit((code, signal) => {
  console.log(`child received ${signal}`)
})

setInterval(() => {}, 1000)

If I run node parent.mjs I expect to see child received SIGINT. Instead I see child received SIGTERM.

geigerzaehler avatar Dec 16 '21 12:12 geigerzaehler

Hi @geigerzaehler,

killSignal is the signal used for terminations due to the timeout, signal or maxBuffer options, not explicit termination with child.kill(). The reason is because specific signals can be passed directly to child.kill(signal).

Note: this is not specific to Execa, i.e. child_process.spawn() has the same behavior.

ehmicky avatar Dec 16 '21 14:12 ehmicky

Thanks for clearing this up @ehmicky and sorry I misunderstood the purpose of killSignal.

The reason for the misunderstanding on my side came from the following use case I have: I want to be able to configure the signal that is used to kill processes when the parent exits. I assumed that I could use killSignal for this but that’s not the case. Concretely, I want the following to work

// parent.mjs
import { execa } from "execa"

const child = execa("node", ["child.mjs"], { killSignal: "SIGINT", stdio: "inherit" })

setTimeout(() => {
  process.exit(0)
}, 100)
// child.mjs
import  onExit from 'signal-exit'

onExit((code, signal) => {
  console.log(`child received ${signal}`)
})

setInterval(() => {}, 1000)

The output should be child received SIGTERM instead of child received SIGTERM.

Happy to open a feature request issue for this and close this issue.

geigerzaehler avatar Dec 16 '21 14:12 geigerzaehler

When a parent process exits, its child processes are terminated by the OS, not by Node.js. Therefore, it is not possible to control which signal is being sent.

(Note: when the child process is detached, Unix does not terminate it (unlike Windows), which is why Execa provides with a cleanup option, but that's a different topic).

I believe the only way for you to control which signal is being sent there would be to explicitly call child.kill(signal).

ehmicky avatar Dec 16 '21 15:12 ehmicky

When a parent process exits, its child processes are terminated by the OS, not by Node.js. Therefore, it is not possible to control which signal is being sent.

I thought the following code was responsible for killing the process if it is not detached: https://github.com/sindresorhus/execa/blob/e4b929547ade335b0dd9d7afe1a35989e19fbc8a/lib/kill.js#L95-L97

The documentation also states, that the cleanup option controls this behavior. I tested it and if I SIGKILL the parent so that the exit hook is not run or set cleanup to false the child stays around. (If you press Ctrl+C in the shell that runs the parent the child will be terminated in any case—but this is due to the shell sending SIGINT to all processes in process group.)

I believe the only way for you to control which signal is being sent there would be to explicitly call child.kill(signal).

Yes, that sounds like a good workaround. But this requires more discipline on the caller side since you need to collect all the processes in an array and then call .kill() on exit. This is what we’re doing here. So a setting that allows us to control the signal send to children on shutdown of the process would make this a lot easier.

geigerzaehler avatar Dec 16 '21 16:12 geigerzaehler

Thanks for the additional context, I see what you mean in terms of complexity from the caller's standpoint.

You're also correct about the cleanup option. To be clear, this option is currently only meant to fix the cross-platform issue I mentioned above, as opposed to as a way to customize the signal being sent.

Introducing an option to customize the signal sent by the kill() of the cleanup option is feasible. However, there might be the following issues limiting the usefulness of that feature:

  • This would only work when detached is false.
  • This would not work when the parent process is crashing or is terminated with SIGKILL.

I am curious what are your thoughts on this @sindresorhus?

ehmicky avatar Dec 16 '21 17:12 ehmicky

I'm not against having a cleanupKillSignal option added, but it's also not something I have time/interest in reviewing a PR for (meaning @ehmicky would need to be willing to review it). It's solving a rather obscure use-case.

sindresorhus avatar Dec 31 '21 23:12 sindresorhus

Revisiting this issue, I think our current implementation of killSignal is inconsistent:

  • It is used by the timeout and signal options.
  • It is not used by the cleanup option, maxBuffer option, .cancel() method. It also not used when the process is killed on stream errors or child process error event.

If a user is specifying a killSignal, it means that signal is the best way to terminate the process. Therefore, it should be used in all the cases above.


A second point is that while child_process.spawn() does not use killSignal as the default value for the .kill() method, I do not see a good reason not to. I think most users would assume this to be the case, which includes @geigerzaehler.

It would also simplify implementing the first point above: instead of passing killSignal in multiple places in the codebase, we can just make it the default value for .kill().


@sindresorhus What do you think about making the killSignal option the default value for the .kill() method's first argument? If neither the killSignal nor the .kill() method's first argument is set (which is probably almost all our users), SIGTERM would still be used.

ehmicky avatar Jan 20 '24 20:01 ehmicky

@sindresorhus What do you think about making the killSignal option the default value for the .kill() method's first argument? If neither the killSignal nor the .kill() method's first argument is set (which is probably almost all our users), SIGTERM would still be used.

👍 As long as it's clearly documented.

sindresorhus avatar Jan 21 '24 08:01 sindresorhus