Unexpected SIGTERM handling #2653
Comments
|
I am not sure why it works differently with pnpm as we use a fork of npm's library for running the scripts. And it has the following code: process.once('SIGINT', procInterupt)
function procInterupt () {
proc.kill('SIGINT')
proc.on('exit', () => {
process.exit()
})
process.once('SIGINT', procKill)
} |
|
@zkochan Here's an example output comparison of npm vs pnpm: `npm run sigterm` gets SIGTERMnpm exits only after it's child is exited, exit code passed. `pnpm run sigterm` gets SIGTERMpnpm exited immediately after forwarding |
|
A general demonstration of the issue with SIGINT:
|
|
Hi, was just wondering if this was being looked at. I've reverted to using |
|
I did add it to a priority list but I have too much to do. And it is pretty hard for me to concentrate now, since I am in Ukraine. I am OK to accept a contribution that makes it work the same way as it works in npm. |
|
Hope you are safe. |
|
I'd be happy to contribute a PR for this if someone can just point me vaguely in the direction of where this is handled in pnpm. Already see the link above for npm. Our team's been slowed down by this one for about a month and we'll have some extra time in the coming month or so to address lower-priority stuff for our project, including resolving this issue. Happy to give back a little to a project that's been instrumental to the success of our own. |
|
The issue is probably in this package: https://github.com/pnpm/npm-lifecycle |
|
Sweet, I'll take a look soon here. |
|
Hey @DylanCulfogienis , are you still working on it? If there's any progress - please share. |
|
@mark-omarov Afraid I haven't gotten to it yet- This work ultimately has to fit around the high-priority work for this project for me to justify it to our client. If someone else stumbles across this and wants to give it a try, you're welcome to! That being said, I will likely have time to take a look at this in late November/December. We're starting to clear out the high priority stuff, and this will be one of the first things I work on after that's said and done. |
|
@DylanCulfogienis thanks for the update. I have tried to figure it out, no luck yet. All I can tell so far is that on termination I see three events fired - It's getting late, but I will take another look sometime later, and give an update if I figure something out. P.S. Update 24/11/2022: |
|
It seems like the issue described in the OP has been solved in the meantime (at least on macOS, can others confirm as well?): ❯ pnpm run sigterm
> monorepo-root@ sigterm <redacted path>
> node sigterm.js
1
1
1
SIGTERM 15
1
1
1
1
1
ELIFECYCLE Command failed with exit code 123.(compare with output in #2653 (comment)) |
|
Yes, it was fixed in v7.18.0 |
|
I am experiencing the broken behaviour that is said to be fixed. Just tried the OP's code to reproduce it and I ended up with a zombie process no matter whether I used pnpm, yarn or npm. Can anybody else confirm this or am I doing something stupid? Versions used: |
|
@ivstiv works fine, but keep in mind this may also depend on your OS |
// package.json
...
"script": {
"sigterm": "node sigterm.js"
}
...// sigterm.js
process.once('SIGTERM', () => {
console.log('SIGTERM');
setTimeout(() => {
process.exit(0);
}, 1_000);
});
process.once('SIGINT', () => {
console.log('SIGINT');
setTimeout(() => {
process.exit(0);
}, 1_000);
});
setInterval(() => {
console.log('>>>');
}, 1_000);Expected ^ Not expected ^ @zkochan appears to remain an issue |
|
I'm still seeing this issue on pnpm 8.10.0 |
extra: this fixes [pnpm issue 2653[(pnpm/pnpm#2653) and see hemerajs#3 as the root cause of why there is this fork.
pnpm version: 5.2.6
Code to reproduce the issue:
pnpm run sigtermkill -TERM <pid_of_pnpm_run_sigterm>Expected behavior:
pnpm run sigtermupon receiving SIGTERM should pass the signal to child and wait for it to exit, pass 123 exit code to parent.Actual behavior:
pnpm run sigtermupon receiving SIGTERM passes signal to child, but does not wait for it to exit, hence child's exit code is lost (and child is potentially running detached?).Additional information:
EDIT:
The text was updated successfully, but these errors were encountered: