Skip to content

Commit 089eefb

Browse files
wiktor-obrebskiwraithgar
authored andcommitted
fix: Revert Signal Handling Regression (#188)
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
1 parent 8a0443b commit 089eefb

File tree

2 files changed

+27
-5
lines changed

2 files changed

+27
-5
lines changed

lib/signal-manager.js

+6-5
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,6 @@
11
const runningProcs = new Set()
22
let handlersInstalled = false
33

4-
// NOTE: these signals aren't actually forwarded anywhere. they're trapped and
5-
// ignored until all child processes have exited. in our next breaking change
6-
// we should rename this
74
const forwardedSignals = [
85
'SIGINT',
96
'SIGTERM',
@@ -12,8 +9,12 @@ const forwardedSignals = [
129
// no-op, this is so receiving the signal doesn't cause us to exit immediately
1310
// instead, we exit after all children have exited when we re-send the signal
1411
// to ourselves. see the catch handler at the bottom of run-script-pkg.js
15-
// istanbul ignore next - this function does nothing
16-
const handleSignal = () => {}
12+
const handleSignal = signal => {
13+
for (const proc of runningProcs) {
14+
proc.kill(signal)
15+
}
16+
}
17+
1718
const setupListeners = () => {
1819
for (const signal of forwardedSignals) {
1920
process.on(signal, handleSignal)

test/signal-manager.js

+21
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,24 @@ test('adds only one handler for each signal, removes handlers when children have
4444

4545
t.end()
4646
})
47+
48+
test('forwards signals to child process', t => {
49+
const proc = new EventEmitter()
50+
proc.kill = (signal) => {
51+
t.equal(signal, signalManager.forwardedSignals[0], 'child receives correct signal')
52+
proc.emit('exit', 0)
53+
for (const forwarded of signalManager.forwardedSignals) {
54+
t.equal(
55+
process.listeners(forwarded).includes(signalManager.handleSignal),
56+
false, 'listener has been removed')
57+
}
58+
t.end()
59+
}
60+
61+
signalManager.add(proc)
62+
// passing the signal name here is necessary to fake the effects of actually
63+
// receiving the signal per nodejs documentation signal handlers receive the
64+
// name of the signal as their first parameter
65+
// https://nodejs.org/api/process.html#process_signal_events
66+
process.emit(signalManager.forwardedSignals[0], signalManager.forwardedSignals[0])
67+
})

0 commit comments

Comments
 (0)