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

Revert Signal Handling Regression #188

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

wiktor-obrebski
Copy link
Contributor

@wiktor-obrebski wiktor-obrebski commented Dec 16, 2023

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:

{
  "name": "npm",
  "scripts": {
    "start": "node ./main-test.js"
  }
}

Create a main-test.js file:

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

Reverting the changes introduced in
commit 545f3be, as it inadvertently
disrupted proper signals support
@wiktor-obrebski wiktor-obrebski requested a review from a team as a code owner December 16, 2023 20:28
@bencripps
Copy link

This seems like a pretty critical issue; it's breaking the graceful shutdown behavior of a lot of our applications; could we get this merged ASAP?

@wraithgar wraithgar merged commit f4534c1 into npm:main Jan 3, 2024
24 checks passed
wraithgar pushed a commit that referenced this pull request Jan 3, 2024
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
@github-actions github-actions bot mentioned this pull request Jan 3, 2024
wraithgar pushed a commit that referenced this pull request Jan 4, 2024
🤖 I have created a release *beep* *boop*
---


## [7.0.3](v7.0.2...v7.0.3)
(2024-01-03)

### Bug Fixes

*
[`089eefb`](089eefb)
Revert Signal Handling Regression (#188) (@wiktor-obrebski)

### Chores

*
[`8a0443b`](8a0443b)
[#187](#187) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`ccf6eb6`](ccf6eb6)
[#187](#187) bump
@npmcli/template-oss from 4.21.1 to 4.21.3 (@dependabot[bot])
*
[`92c4354`](92c4354)
[#184](#184) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`1f5fd9a`](1f5fd9a)
[#184](#184) bump
@npmcli/template-oss from 4.19.0 to 4.21.1 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 this pull request may close these issues.

3 participants