Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the ‘forceKillAfterTimeout’ option to be passed to ‘.cancel()’ #511

Closed
leafac opened this issue Nov 26, 2022 · 12 comments · Fixed by #714
Closed

Allow the ‘forceKillAfterTimeout’ option to be passed to ‘.cancel()’ #511

leafac opened this issue Nov 26, 2022 · 12 comments · Fixed by #714

Comments

@leafac
Copy link
Contributor

leafac commented Nov 26, 2022

The documentation says we should prefer .cancel() over .kill(), so it would be nice for .cancel() to accept the forceKillAfterTimeout option that .kill() has.

@sindresorhus
Copy link
Owner

We are not going to add more to .cancel() as it's the legacy way to cancel the process. The modern way is with the signal option. We could consider an option to execa() to force kill when after a timeout when aborting.

@leafac
Copy link
Contributor Author

leafac commented Nov 26, 2022

@sindresorhus Oh, that’s good to know. Thank you very much for the answer. Two follow-up questions:

  1. Do you want me to keep this issue open for “We could consider an option to execa() to force kill when after a timeout when aborting.”? Or do you want me to open a new one? Or no issue at all?

  2. Would you consider communicating the fact that .cancel() (and presumably .kill()) is legacy in the documentation? I’d be happy to send a pull request for that if you wish. Specifically, there are two places that could be changed:

    1. Remove this section of the README.
    2. The documentation in the types could mention that you should be using signal instead.

@sindresorhus
Copy link
Owner

  1. We can keep this one open.
  2. Yes, we should do that. .kill() is not legacy, only .cancel().

@leafac
Copy link
Contributor Author

leafac commented Dec 5, 2022

@sindresorhus

  • Yes, we should do that. .kill() is not legacy, only .cancel().

Please consider #512

@ehmicky
Copy link
Collaborator

ehmicky commented Jan 20, 2024

There is a good point raised in this issue: we have a graceful termination feature, which is pretty useful, and can be controlled with the forceKillAfterTimeout option. However, it does not integrate well with our other features. Namely, the following cannot specify the forceKillAfterTimeout:

  • signal option
  • cleanup option
  • timeout option
  • maxBuffer option
  • stream errors
  • childProcess.on('error') event
  • .cancel() method

I would suggest the following solution: move the forceKillAfterTimeout option from the .kill() method.

const childProcess = execa('...')
childProcess.kill('SIGTERM', {forceKillAfterTimeout: 2000})

To the process instantiation instead.

const childProcess = execa('...', {forceKillAfterTimeout: 2000})
childProcess.kill()

This would enable the other features to use it.

I do not think there is a strong use case for allowing different timeout values for different kill() calls. The duration of graceful exit is usually determined by how long it takes for a child process to cleanup. This is not something the parent process controls. Therefore, it usually does not change after spawning the process.

On the other hand, there is a use case for a parent process to terminate a child process sometimes gracefully, sometimes not. But that can be done by using different signals.

const childProcess = execa('...', {forceKillAfterTimeout: 2000})
const killSignal = graceful ? 'SIGTERM' : 'SIGKILL'
childProcess.kill(killSignal)

What do you think @sindresorhus?

@sindresorhus
Copy link
Owner

Agreed 👍

@ehmicky
Copy link
Collaborator

ehmicky commented Jan 22, 2024

What are your thoughts on taking that opportunity to rename that option to gracefulExit instead?
If you prefer the current name, this works for me too. 👍

@sindresorhus
Copy link
Owner

gracefulExit implies a smooth, unforced process termination, which doesn't accurately convey the forceful aspect of SIGKILL.

@ehmicky
Copy link
Collaborator

ehmicky commented Jan 23, 2024

That's right. An actual graceful exit would be not to use SIGKILL at all.
So what's we are doing here is actually not a graceful exit, but preferring a forceful exit over no exit at all.

What about forceExit instead?
I am wondering whether some users might confuse this option with the timeout option if the name includes that word. What do you think?

@sindresorhus
Copy link
Owner

forceExit makes it sound like it does force exit no matter what. We could change it to forceKillAfterDelay, which is a bit more accurate. Or forceKillOnHang.

@ehmicky
Copy link
Collaborator

ehmicky commented Jan 23, 2024

forceKillAfterDelay sounds good 👍

@ehmicky
Copy link
Collaborator

ehmicky commented May 8, 2024

This feature has been just released in Execa 9.0.0. Please see (and share!) the release post and the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants