This repository has been archived by the owner. It is now read-only.

lifecycle: propagate SIGTERM to child #10868

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
8 participants
@daniel-pedersen

When one runs for example npm run-script, any SIGTERM to npm will orphanize the child process. This is a simple fix for this (potential) issue. Now whenever npm launches a child through lifecycle, any SIGTERM signal is simply propagated to the child for the duration of the process.

This behavior of npm when getting a SIGTERM seems to be what makes sense to most people (see the discussion in #4603), and makes npm run-script more friendly in various development scenarios.

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Jan 6, 2016

Member

This looks pretty reasonable to me. It does still need a test.

Member

iarna commented Jan 6, 2016

This looks pretty reasonable to me. It does still need a test.

@iarna iarna added the needs-tests label Jan 6, 2016

@daniel-pedersen

This comment has been minimized.

Show comment
Hide comment
@daniel-pedersen

daniel-pedersen Feb 12, 2016

Done. The new test fails and hangs without previous commit, and the commit makes it pass.

Done. The new test fails and hangs without previous commit, and the commit makes it pass.

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Feb 25, 2016

Member

Awesome, thank you!

Member

iarna commented Feb 25, 2016

Awesome, thank you!

@iarna iarna added merge-to-latest and removed needs-tests labels Feb 25, 2016

@iarna iarna added this to the next-next milestone Feb 25, 2016

@zkat

This comment has been minimized.

Show comment
Hide comment
@zkat

zkat Mar 2, 2016

Member

I was considering whether to pull this into the 2.x LTS, and came to the conclusion that while folks may really not want to have orphaned zombies just from a ^C, there is, in fact, a chance that someone out there is already relying on this behavior.

Because of that, and because LTS is supposed to be more careful about stuff like this, I'm not gonna merge this into LTS. If anyone has other thoughts or objections, please feel free to chime in, since it's not set in stone. :)

Member

zkat commented Mar 2, 2016

I was considering whether to pull this into the 2.x LTS, and came to the conclusion that while folks may really not want to have orphaned zombies just from a ^C, there is, in fact, a chance that someone out there is already relying on this behavior.

Because of that, and because LTS is supposed to be more careful about stuff like this, I'm not gonna merge this into LTS. If anyone has other thoughts or objections, please feel free to chime in, since it's not set in stone. :)

iarna added a commit that referenced this pull request Mar 4, 2016

@iarna

This comment has been minimized.

Show comment
Hide comment
@iarna

iarna Mar 4, 2016

Member

This landed in 3.8.1!

Member

iarna commented Mar 4, 2016

This landed in 3.8.1!

@iarna iarna closed this Mar 4, 2016

Gobd referenced this pull request in weekly-deals/whats-your-dealio Apr 18, 2016

@gaastonsr

This comment has been minimized.

Show comment
Hide comment

Freaking awesome @daniel-pedersen

@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers May 5, 2016

Hey guys, is this known/expected to work on Linux? I can see that it works on MacOS, but it doesn't seem to be working on our UAT box which is running Ubuntu Server?

Hey guys, is this known/expected to work on Linux? I can see that it works on MacOS, but it doesn't seem to be working on our UAT box which is running Ubuntu Server?

segrey added a commit to segrey/npm that referenced this pull request Jun 21, 2016

@kucharskimaciej

This comment has been minimized.

Show comment
Hide comment
@kucharskimaciej

kucharskimaciej Jun 30, 2016

Seconding @dchambers on this. Make for quite annoying experience when automating tasks with npm.

Seconding @dchambers on this. Make for quite annoying experience when automating tasks with npm.

@twang2218

This comment has been minimized.

Show comment
Hide comment
@twang2218

twang2218 Dec 12, 2016

I'm using docker with npm start, and the npm doesn't forward the signal to child-processes, which caused graceful shutdown failed, and the app process be SIGKILLed instead, without a chance to release the resources.

Are you sure this PR is working on Linux? My NPM version is 3.10.10

$ npm -v
3.10.10
$ node -v
v7.2.1

I'm using docker with npm start, and the npm doesn't forward the signal to child-processes, which caused graceful shutdown failed, and the app process be SIGKILLed instead, without a chance to release the resources.

Are you sure this PR is working on Linux? My NPM version is 3.10.10

$ npm -v
3.10.10
$ node -v
v7.2.1
@dchambers

This comment has been minimized.

Show comment
Hide comment
@dchambers

dchambers Dec 12, 2016

Hey @twang2218, it may be worth creating a new issue specifically for Linux support as this has been closed quite some time now.

Hey @twang2218, it may be worth creating a new issue specifically for Linux support as this has been closed quite some time now.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 29, 2017

Contributor

Yes, this isn’t working, at least with bash; npm runs its lifecycle processes in a shell, and bash doesn’t forward SIGTERM to its children.

Contributor

addaleax commented Apr 29, 2017

Yes, this isn’t working, at least with bash; npm runs its lifecycle processes in a shell, and bash doesn’t forward SIGTERM to its children.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.