Skip to content

Commit

Permalink
Revert Signal Handling Regression (#188)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
wiktor-obrebski authored Jan 3, 2024
1 parent 8a0443b commit f4534c1
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 5 deletions.
11 changes: 6 additions & 5 deletions lib/signal-manager.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
const runningProcs = new Set()
let handlersInstalled = false

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

const setupListeners = () => {
for (const signal of forwardedSignals) {
process.on(signal, handleSignal)
Expand Down
21 changes: 21 additions & 0 deletions test/signal-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,24 @@ test('adds only one handler for each signal, removes handlers when children have

t.end()
})

test('forwards signals to child process', t => {
const proc = new EventEmitter()
proc.kill = (signal) => {
t.equal(signal, signalManager.forwardedSignals[0], 'child receives correct signal')
proc.emit('exit', 0)
for (const forwarded of signalManager.forwardedSignals) {
t.equal(
process.listeners(forwarded).includes(signalManager.handleSignal),
false, 'listener has been removed')
}
t.end()
}

signalManager.add(proc)
// passing the signal name here is necessary to fake the effects of actually
// receiving the signal per nodejs documentation signal handlers receive the
// name of the signal as their first parameter
// https://nodejs.org/api/process.html#process_signal_events
process.emit(signalManager.forwardedSignals[0], signalManager.forwardedSignals[0])
})

0 comments on commit f4534c1

Please sign in to comment.