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

Fix process not recycled properly on Windows #1727

Merged
merged 3 commits into from
Nov 2, 2015
Merged

Conversation

kflu
Copy link
Contributor

@kflu kflu commented Oct 29, 2015

This fixes #1664

@kflu
Copy link
Contributor Author

kflu commented Oct 29, 2015

what should I do with that travis ci failure? I think it's not related to my change.

@soyuka
Copy link
Collaborator

soyuka commented Oct 30, 2015

Don't remove the - it's to kill the tree! You should rather add a condition for windows. Condition that will end up using treeKill for example, like it's done with the cluster.

@kflu
Copy link
Contributor Author

kflu commented Oct 31, 2015

@soyuka Thanks for the info. I opened an issue against Node (nodejs/node/issues/3617)for its not being consistent across platforms. Meanwhile, I provided this fix as a work around on Windows.

@@ -242,7 +257,7 @@ module.exports = function(God) {
if (mode.indexOf('cluster') === 0)
treekill(parseInt(pid));
else
process.kill(-(parseInt(pid)), 'SIGINT');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my knowledge, why the behaviors differ in terms of mode.indexOf('cluster') ? In other word, why couldn't we use treekill instead of process.kill(-pid) everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I explained this in #1036 (comment). Basically, cluster doesn't accept a {detached: true} option (like fork) and causes it no to be attached to it's own child processes.

See also this test case : https://gist.github.com/soyuka/90378a78b317ecc7b2c3 (I could translate if you need me to)

@soyuka
Copy link
Collaborator

soyuka commented Oct 31, 2015

#1036 feature introduction

@Unitech
Copy link
Owner

Unitech commented Nov 2, 2015

Could you please send this PR on development branch? Thanks!

Unitech added a commit that referenced this pull request Nov 2, 2015
Fix process not recycled properly on Windows
@Unitech Unitech merged commit 21a03ff into Unitech:master Nov 2, 2015
@kflu
Copy link
Contributor Author

kflu commented Nov 2, 2015

@Unitech I wasn't familiar with the work flow. Looks like you've merged it directly into master. Do you still need me to send it to development branch?

@Unitech
Copy link
Owner

Unitech commented Nov 2, 2015

Yes I merged it and it's already available on the latest PM2 0.15.9 (published today)

Thanks!

@kflu
Copy link
Contributor Author

kflu commented Nov 2, 2015

Awesome. Thanks!

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.

pm2 stop does not kill app process on Windows 7
3 participants