pm2 now kills detached processes (as of 0.12.2)

[ryanbmilbourne]

I have pm2 managing a node daemon that spawns worker processes as detached children. The idea was that if the daemon was killed or restarted (after an upgrade, for example) the workers would continue working and publishing to the pub/sub system they work with. After upgrading to the latest version from 0.10.x, I noticed that pm2 was killing my detached workers as well as the daemon it was managing.

It looks to me like this is directly related to the addition of the new TreeKill system (fe92e71) in 0.12.2. See lines 69-70 of TreeKill:

  var args = downgradePs ? '-eo pid,ppid | grep -w ' : '-o pid --no-headers --ppid ';
  var ps = exec('ps ' + args + ppid, function(err, stdout, stderr){...}

That ps command will return the list of all children with that ppid, which, unfortunately, includes children that are detached so that they can live beyond the death of the parent that spawned them. Rather than sending a signal to the parent and relying on it to clean up the children, it looks like pm2 is now sending SIGTERM to all processes in the tree.

Would there be support for an option or config to have pm2 kill processes in the older manner as opposed to this new(er) KillTree method? It seems to me like there'd be support to kill the parent and have it clean up its children as it sees fit rather than use the nuclear option.

[ryanbmilbourne]

On further inspection, the versioning of the package json doesn't match the commit history. The package.json commits indicate that treeKill was added around 0.11.0, but the change history indicates the change at 0.12.2.

[Unitech]

Have you tried to do the exec in a really detached way? So these process would not have any link with their parents? We totally detach PM2 from the CLI here: https://github.com/Unitech/PM2/blob/master/lib/Satan.js#L334 if it can be of any help

[ryanbmilbourne]

I use detached: true but the detached process still lists the parent's pid as the ppid value in the ps utility. Since KillTree uses ps to build its tree, it groups in the detached children. I also use child.unref().

0.10.8 works just fine, 0.11.0 does not.

[soyuka]

@Unitech can't we add an option to prevent killing the whole tree?

I've tried this out and indeed detached process is beeing killed along with the main process. See https://gist.github.com/soyuka/ca813a8baf1c8efb7974

[Unitech]

Ok if it's impossible to detach completely a process to avoid killing it, this option worth it

[ryanbmilbourne]

@Unitech I think a good way forward would be to allow this to be configured at the application level:
$pm2 start test.js --killTree=false
...or in the app's json config file. This way you can keep the default behavior you have now but create exceptions for certain applications to only kill the parent and let the OS determine how the children handle that. This would support both behaviors and keep both folks happy.

Thoughts?

[jshkurti]

@ryanbmilbourne That's exactly what we were going to do :)

[soyuka]

I think that we should never kill detached processes, that's why they are detached in a way...

See my commit, we could easily get rid of TreeKill, or it could be a non-default behavior to kill detached processes along with the main process.

At the current status of the --no-treekill option, pm2 will kill child process without killing detached ones.

We could also bring a way to never kill any childs (process.kill(pid, 'SIGTERM')). But, is there really a point to this? If we wanted them not to be killed, we could simply detach them.

Thoughts?

[ryanbmilbourne]

Personally, I'm of the opinion that we should kill the parent and let the OS decide how the children live or die. process.kill(pid, 'SIGTERM') should kill any non-detached children as well as the parent.

Using a blunderbuss like TreeKill to kill children seems a little...over the top, as it kills ALL children regardless of how they're tied to the parent. I agree with @soyuka that the tree kill should be the non-default.

[soyuka]

@ryanbmilbourne I agree with you. I made a new branch with a simple and cleaner solution that will kill the tree except detached child processes.

[ryanbmilbourne]

👍 @soyuka Any hope of that being pushed into npm anytime soon?

[jshkurti]

Yes it will be published soon :)

[soyuka]

So, @ryanbmilbourne I have this working with the fork mode but I have an issue with the cluster.

• When I execute a process without pm2 in a cluster they are in the same TTY with the same group id:

 501  5504  5500   0  8:36   ??         0:00.00 tail -F ./childrensdetached.log
 501  5505  5500   0  8:36   ttys023    0:00.00 tail -F ./childrens.log
 501  5499  4354   0  8:36   ttys023    0:00.08 node index.js

• With pm2, TTY seems to be detached and so are child processes:

 501 30231 30230   0 10:48   ??         0:00.22 /Users/soyuka/.nvm/versions/node/v0.12.0/bin/node --debug-port=5859 /Users/soyuka/forks/pm2/lib/ProcessContainer.js
 501 30244 30231   0 10:48   ??         0:00.02 tail -F ./childrensdetached.log
 501 30246 30231   0 10:48   ??         0:00.02 tail -F ./childrens.log

This sucks, I'm searching for a solution but it might take a few hours debugging. I don't know if I'll be able to fix it but I'll surely try!
In the mean time you can use my branch but be warned it won't work at all with cluster and might break a lot of things ^^.

[ryanbmilbourne]

Great, thank you so much for the quick turnaround. I'll modify my modules' package.json files to look to your repo for the dependency until the PR issue is resolved and the code is merged into the release branch.

[soyuka]

So, little update on this.

The cluster mode is using fork internally but won't allow us to pass any options like detached: true. The fact is that usually the master is part of the cluster, so childrens are attached to the master. But in our case, pm2 is the master and it's cluster childrens should not be attached to the master process (here pm2).

If we could build our own cluster we would be able to fix this but so far, no solution on the cluster mode.

If you want I can implement it for the fork_mode.

[ryanbmilbourne]

@soyuka Any update on your issues with cluster? We were still hoping to see this fix get merged in eventually...

[soyuka]

Sadly we can't have a clean fix for the cluster_mode because the nodejs cluster module is not calling the childrens in a detached state.

[jshkurti]

@ryanbmilbourne If your app is running in fork mode, we can easily fix that like this :

if (app.mode == 'cluster')
  treeKill(pid);
else
  process.kill(-(pid), 'SIGINT');

[jshkurti]

1915fdd

[soyuka]

My two cents: treeKill isn't a pretty nor portable way of killing the child processes. Anyway process.kill(-pid) will do a perfect job with the fork_mode.

[ryanbmilbourne]

Our app is running in fork_mode, so we'd be happy with that fix. For now it sounds like fix isn't in the works for cluster_mode, but that doesn't hinder us at all.

[jshkurti]

You can already use it npm i -g Unitech/PM2#development :)
The official release will be published soon.

[ryanbmilbourne]

Great, I'll get the dev branch running on our test servers. Thanks for the awesome project.

[soyuka]

@jshkurti What about adding an option to not kill child processes?

[brandonros]

Is this really still an issue, 4 years later?

Edit: pass killtree in ecosystem config file

[Cat7373]

Is this really still an issue, 4 years later?

Edit: pass killtree in ecosystem config file

实际上，我找到了他们的解决方案：https://github.com/Unitech/pm2/blob/master/examples/treekill/process.json

但这太 shit 了（原谅我，毕竟这浪费了我太多心力），官方文档上似乎没有任何地方提到此特性，更没有提到这个配置，我整整找了四个小时，才找到这里，还得在全仓库搜索关键字才找到这个配置

[chenliq]

Is this really still an issue, 4 years later?
Edit: pass killtree in ecosystem config file

实际上，我找到了他们的解决方案：https://github.com/Unitech/pm2/blob/master/examples/treekill/process.json

但这太 shit 了（原谅我，毕竟这浪费了我太多心力），官方文档上似乎没有任何地方提到此特性，更没有提到这个配置，我整整找了四个小时，才找到这里，还得在全仓库搜索关键字才找到这个配置

今天我上官网文档还是没有看到配置项有这一条，我用google搜索关键字：pm2 stop kill sub process，在stack overflow 找到了答案