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

[BUG] npm not handling process signals #6684

Open
2 tasks done
ryanrasti opened this issue Jul 28, 2023 · 12 comments
Open
2 tasks done

[BUG] npm not handling process signals #6684

ryanrasti opened this issue Jul 28, 2023 · 12 comments
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release

Comments

@ryanrasti
Copy link

ryanrasti commented Jul 28, 2023

Is there an existing issue for this?

  • I have searched the existing issues

NOTE: I originally posted this issue on nodejs/node but was redirected here.

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

npm doesn't handle SIGTERM like it used to. I noticed this issue specifically when updating node from v20.2.0 to v20.3.0 (npm 9.6.7 to 9.8.1).

Expected Behavior

Expected that the below program exits with code 0 (i.e., npm is aborted by the signal and the script handles the signal as well)

Steps To Reproduce

The below script sends a SIGTERM to the npm process that spawned it. Run npm run test with:

In package.json

    {
      "name": "test",
      "version": "0.0.0",
      "type": "module",
      "scripts": {
        "test": "node test.js"
      }
    }

In test.js

    import { execSync } from 'child_process';

    process.on('SIGTERM', () => {
        console.log('Received SIGTERM. Exiting...');
        process.exit(0);
    });

    const output = execSync('ps -e | grep npm').toString().split(/\s+/).filter(Boolean);
    const pid = Number(output[0]);
    process.kill(pid)

    setTimeout(() => {
        console.log('Fatal: Signal was not received. Exiting... ');
        process.exit(1)
    }, 2000);

Environment

  • npm: 9.8.0
  • Node.js: v20.5.0
  • OS Name: Linux nixos 5.15.93 x86_64 GNU/Linux
  • System Model Name:
  • npm config:
; node bin location = /nix/store/1gnvy2dhh311c900hzyw6ppjdhnir2s5-nodejs-20.5.0/bin/node
; node version = v20.5.0
; npm local prefix = /build
; npm version = 9.8.0
; cwd = /build
; HOME = /homeless-shelter
; Run `npm config ls -l` to show all defaults.
@ryanrasti ryanrasti added Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release labels Jul 28, 2023
@ngbrown
Copy link

ngbrown commented Jul 28, 2023

@ryanrasti I'm getting this same behavior in npm v9.6.7. Can you verify what npm version was working?

In my case package.json scripts that were receiving the signals fine in npm v8.19.4 (node v16.20.1) stopped working when upgrading directly to npm v9.6.7 (node v18.17.0).

There is a possibly identical issue that was opened at #6547, but no triage occurred in the month since it was opened.

This may be caused by a change made in npm/run-script#142. There were no tests associated with the changed behavior. Maybe an issue needs opened in that repository?

cc: @nlf, @wraithgar

@ryanrasti
Copy link
Author

Indeed, good catch @ngbrown , the version that worked for me was 9.6.6 (not 9.6.7).

Indeed, this looks like a duplicate of #6547 but that bug is incorrectly tagged with Release 8.x

@thejohnfreeman
Copy link

thejohnfreeman commented Aug 21, 2023

How does this issue still have zero response? Very disappointing. @nlf @wraithgar

For those looking for a workaround on Linux, you can kill the entire process tree like this:

npm start &
pid=$! # root process ID
gid=$(ps -o pgid= $pid) # process group ID
kill -SIGTERM -$gid

As a bonus, you can inspect the process tree with either the root process ID or the process group ID like this:

pstree -pT $pid
ps -jH -$gid

@dmaizel
Copy link

dmaizel commented Oct 18, 2023

Any news about this?

wraithgar pushed a commit to npm/run-script that referenced this issue Jan 3, 2024
## 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
wraithgar pushed a commit to npm/run-script that referenced this issue 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
@tdislay
Copy link

tdislay commented Feb 7, 2024

Since [email protected] which depends of @npm/[email protected] (in which the regression was fixed), everything works fine for me :).
Thanks 🎉

@mahnunchik
Copy link

I've faced the same issue...

@DesignByOnyx
Copy link

I can confirm that npm version 10.3.0 does forward the signals as expected.

@alexrus123
Copy link

I think this is still happening on Ubuntu 20.04.6 LTS with node 18.20.0 and npm 10.5.0

@Dzieni
Copy link

Dzieni commented Apr 24, 2024

We have tested 10.3.0 and 10.5.0 on Node 21.7.3 & MacOS Sonoma and unfortunately the problem is still there.

@ekeren
Copy link

ekeren commented Apr 24, 2024

I am able to reproduce on npm 10.3.0 and node v21.6.2, on macos 13.4

@skyrpex
Copy link

skyrpex commented Apr 24, 2024

Here a small repository with another example that reproduces the problem: https://github.com/skyrpex/npm-scripts-sigint-problem.

The issue happens with npm 10.5.2.

@DesignByOnyx
Copy link

I have created a demo repo to showcase the bug. The repo verifies that this only happens in versions of NPM >=9.6.7 <10.3.0. If you experience this in another version of NPM, please create a repo which shows the error happening.

https://github.com/DesignByOnyx/npm-signals-bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 9.x work is associated with a specific npm 9 release
Projects
None yet
Development

No branches or pull requests