Improve graceful exit #714
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ehmicky
commented
Jan 22, 2024
| /** | ||
| Same as the original [`child_process#kill()`](https://nodejs.org/api/child_process.html#child_process_subprocess_kill_signal), except if `signal` is `SIGTERM` (the default value) and the child process is not terminated after 5 seconds, force it by sending `SIGKILL`. Note that this graceful termination does not work on Windows, because Windows [doesn't support signals](https://nodejs.org/api/process.html#process_signal_events) (`SIGKILL` and `SIGTERM` has the same effect of force-killing the process immediately.) If you want to achieve graceful termination on Windows, you have to use other means, such as [`taskkill`](https://github.com/sindresorhus/taskkill). | ||
| */ | ||
| kill(signal?: string, options?: KillOptions): void; |
There was a problem hiding this comment.
process.kill() now has the same signature as in core Node.js.
The only difference is the graceful exit behavior, which we document with the forceKillAfterTimeout option, so there's no need to document process.kill() here anymore.
ehmicky
commented
Jan 22, 2024
| forceKillAfterTimeout: 2000 | ||
| }); | ||
| }, 1000); | ||
| ``` |
There was a problem hiding this comment.
The graceful exit behavior happens by default. It's probably not critical to add an example for it anymore. This leaves more room for other more useful examples.
ehmicky
force-pushed
the
graceful-exit
branch
from
92a8a3d
to
b597e9f
Compare
ehmicky
commented
Jan 22, 2024
| @@ -6,6 +6,7 @@ process.on('SIGTERM', () => { | |||
| }); | |||
|
|
|||
| process.send(''); | |||
| console.log('.'); | |||
There was a problem hiding this comment.
Needed for the maxBuffer test.
ehmicky
force-pushed
the
graceful-exit
branch
from
b597e9f
to
382af21
Compare
ehmicky
commented
Jan 22, 2024
ehmicky
changed the title
forceKillAfterTimeout options to top-level
ehmicky
force-pushed
the
graceful-exit
branch
4 times, most recently
from
88179ee
to
175c0de
Compare
ehmicky
commented
Jan 22, 2024
| await pSetTimeout(timeout, undefined, {ref: false}); | ||
| kill('SIGKILL'); | ||
| try { | ||
| await setTimeout(forceKillAfterTimeout, undefined, {signal: controller.signal}); |
There was a problem hiding this comment.
The controller is aborted when the child process exits (successfully or not). When this happens, setTimeout() throws, hence the empty catch block.
ehmicky
commented
Jan 22, 2024
| @@ -43,20 +45,18 @@ const getForceKillAfterTimeout = (signal, forceKillAfterTimeout, killResult) => | |||
| }; | |||
|
|
|||
| const killAfterTimeout = async ({spawned, timeout, killSignal, context, controller}) => { | |||
| await pSetTimeout(timeout, undefined, {ref: false, signal: controller.signal}); | |||
There was a problem hiding this comment.
ref: false not needed thanks to the controller.
ehmicky
force-pushed
the
graceful-exit
branch
from
175c0de
to
68c3739
Compare
sindresorhus
requested changes
Jan 23, 2024
readme.md
Outdated
| @@ -50,7 +50,7 @@ This package improves [`child_process`](https://nodejs.org/api/child_process.htm | |||
| - Redirect [`stdin`](#stdin)/[`stdout`](#stdout-1)/[`stderr`](#stderr-1) from/to files, streams, iterables, strings, `Uint8Array` or [objects](docs/transform.md#object-mode). | |||
| - [Transform](docs/transform.md) `stdin`/`stdout`/`stderr` with simple functions. | |||
| - Iterate over [each text line](docs/transform.md#binary-data) output by the process. | |||
| - [Graceful termination](#optionsforcekillaftertimeout). | |||
| - [Graceful termination](#forcekillaftertimeout). | |||
There was a problem hiding this comment.
We should use a better word than "graceful" as it can be misunderstood.
ehmicky
force-pushed
the
graceful-exit
branch
from
7312082
to
05f8606
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #511.
This extends graceful exits to the following termination methods:
signaloptioncleanupoptiontimeoutoptionmaxBufferoptionchildProcess.on('error')eventThis moves the
forceKillAfterTimeoutoption from the.kill()method to the top-level options instead.