fix: handle signals more correctly #142
Conversation
previously we were assuming that npm received the signal first and was required to forward the signal to child processes, however that seems to no longer be the case. instead npm and the child process both receive the signal at the same time. the previous logic has been modified such that it places a no-op function as the signal handler. this is strictly to prevent the default behavior of exiting node from happening. once all child process have exited, the handlers are all removed and we exit appropriately
|
it may be worth taking the time to write an integration test for this. it would have to spawn a process that spawns another process and send signals in and assert both processes exit.. it sounds pretty hairy so i haven't done it yet, but it would be a good way to ensure this doesn't cause a regression in signal handling |
|
Now npm doesn't forward signals to the chi;d process. |
|
@nlf Now it is impossible to terminalte process using SIGTERM. Yhis is serious issue, which leads to the data loss in many applications. Could you please restorre old functionality? |
|
@nlf |
|
@nlf https://github.com/zdm/npm-issue-547 just clone it and run As you can see it is now not possible to terminate process with the |
|
@nlf Are you alive? Could you answer please? |
| const handleSignal = signal => { | ||
| for (const proc of runningProcs) { | ||
| proc.kill(signal) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this assumes npm is the process group leader and that signals to npm's pid therefore get propagated to all other pids in the group.
That's a bad assumption though. It's a breaking - and, I suspect, unintentional - change for processes that switch process groups. Those stay behind now.
There was a problem hiding this comment.
I think the author might have confused terminating the program with CTRL + C (which still works) with sending a SIGTERM signal, for example, by using kill -SIGTERM $PID. This particular functionality seems to have been broken by this commit.
## Description This PR reverts the changes made in commit `545f3be94d412941537ad0011717933d48cb58cf`, which inadvertently broke signal forwarding to child processes (PR #142 ). Contrary to the assumptions by @nlf , `SIGTERM` and similar signals are not being correctly propagated to child processes. Instead, they are only received by npm, resulting in incomplete signal handling. The removal of signal forwarding in #142 means that child processes do not receive necessary signals for appropriate cleanup and termination. This issue is evident in workflows involving `npm start` used as a Docker command for local execution. For instance, using CTRL + C does not properly terminate the application and results in a forced kill after a 10-second delay. This behavior could lead to more significant problems in production environments, (if `npm` is used to start the app) such as data loss due to improper database connection closures. ## Minimal Reproduction Steps Create a package.json with the following content: ```json { "name": "npm", "scripts": { "start": "node ./main-test.js" } } ``` Create a main-test.js file: ```typescript const interval = setInterval(() => console.log('alive!'), 3000); async function onSignal(signal) { console.log(`${signal} received, cleaning up...`); clearInterval(interval); console.log('Cleaning up done'); } process.on('SIGINT', onSignal); process.on('SIGTERM', onSignal); ``` Execute `npm start`. The script should output `alive!` every 3 seconds. Attempt to terminate it using `kill -SIGTERM [PID]`. It should log `Cleaning up done` and shut down gracefully, which it does in older versions of `npm` (e.g., `v8.19.4`) but fails in newer versions (e.g., `v9.6.7`). ## Impact Reverting this change will restore the expected behavior for signal handling in `npm` # References - npm/cli#6547 - npm/cli#6684 - #142
This PR reverts the changes made in commit `545f3be94d412941537ad0011717933d48cb58cf`, which inadvertently broke signal forwarding to child processes (PR #142 ). Contrary to the assumptions by @nlf , `SIGTERM` and similar signals are not being correctly propagated to child processes. Instead, they are only received by npm, resulting in incomplete signal handling. The removal of signal forwarding in #142 means that child processes do not receive necessary signals for appropriate cleanup and termination. This issue is evident in workflows involving `npm start` used as a Docker command for local execution. For instance, using CTRL + C does not properly terminate the application and results in a forced kill after a 10-second delay. This behavior could lead to more significant problems in production environments, (if `npm` is used to start the app) such as data loss due to improper database connection closures. Create a package.json with the following content: ```json { "name": "npm", "scripts": { "start": "node ./main-test.js" } } ``` Create a main-test.js file: ```typescript const interval = setInterval(() => console.log('alive!'), 3000); async function onSignal(signal) { console.log(`${signal} received, cleaning up...`); clearInterval(interval); console.log('Cleaning up done'); } process.on('SIGINT', onSignal); process.on('SIGTERM', onSignal); ``` Execute `npm start`. The script should output `alive!` every 3 seconds. Attempt to terminate it using `kill -SIGTERM [PID]`. It should log `Cleaning up done` and shut down gracefully, which it does in older versions of `npm` (e.g., `v8.19.4`) but fails in newer versions (e.g., `v9.6.7`). Reverting this change will restore the expected behavior for signal handling in `npm` - npm/cli#6547 - npm/cli#6684 - #142
previously we were assuming that npm received the signal first and was required to forward the signal to child processes, however that seems to no longer be the case. instead npm and the child process both receive the signal at the same time. the previous logic has been modified such that it places a no-op function as the signal handler. this is strictly to prevent the default behavior of exiting node from happening. once all child process have exited, the handlers are all removed and we exit appropriately