search

pkrumins / node-tree-kill

kill trees of processes
MIT License
333 stars 37 forks source link

race condition #7

Closed jub3i closed 8 years ago

jub3i commented 9 years ago

It can happen that ps emits the exit event, but ps.stdout.on('data', cb) has not yet emptied the stream into allData. This happens because exit event "stdio streams might still be open." So the result is that a call to the exported function just ends up hanging sometimes because buildProcessTree() is not called again.

I've made a patched version relying on ps.stdout.on('close', cb) event instead.

jub3i commented 9 years ago

I think pkrumins is happy with this module, but if anyone else is interested in patched version see: https://www.npmjs.com/package/@jub3i/tree-kill and https://github.com/jub3i/tree-kill

jub3i commented 9 years ago

@wmhilton i see you are breathing new life into this repo! I would happily prepare a PR to port the fixes into this repo? Since you merged callbacks this is now directly relevant.

billiegoose commented 9 years ago

I'd love it! Yes, pkrumins is busy with start-up work, so he asked if I'd be co-maintainer. I'll be trying to close any open issues and accept pull requests in the next couple days. If you can rebase your fix on the latest master branch that'd be great!

jub3i commented 9 years ago

Ok so I prepared a branch issue#7-alt which relies on stdout 'close' event as I initially described.

Im not too fond of this solution because there is a lot of extra code. First off you have to check the initial pid is an integer, because the stdout 'close' event doesn't have an exit code in the callback. This means at line 78 we must compare against allData === '' and if the initial pid is something other than an integer then ps will display a help message, and then the program will try to continue building a process tree out of that help text.

The solution I prefer is branch issue#7 which simply removes the 'exit' event handler. I'm kind of nervous about this solution though, because @pkrumins attached the 'exit' event handler for some reason. However, from what I can tell from line 58 we can safely rely on ps 'close' event because its stdio stream are always its own and not shared with other processes, as the default it to pipe its stdio streams to its parent (see node docs and here). I think via Occam’s Razor this is the better solution, and after testing in some of my programs I have not had any problems with it.

In both cases the dependency on once is no longer needed, and in both branches I removed the superfluous execution bit from all the files.

billiegoose commented 8 years ago

I know it's been freaking forever (how did four months go by that fast?) but the fix from May 15 is now in "master" branch and present in the latest node-tree-kill release on NPM.