darkguy2008 / parallelshell

Run multiple shell commands in parallel
501 stars 44 forks source link

`npm run` + OSX -- error doesn't kill all processes #22

Open nichoth opened 9 years ago

nichoth commented 9 years ago

I made a repo to demonstrate the issue:

https://github.com/nichoth/para-test

This is a common scenario for me—working on multiple modules that use the same port. In one terminal, run parallelshell "node server.js" "watchify ...". In a second terminal, run the same command, and the server emits an error but watchify still runs. This is using npm run scripts.

pklingem commented 9 years ago

+1 same issue here, have you had any luck working around this issue?

nichoth commented 9 years ago

It happens rarely enough I haven't tried fixing.

keithamus commented 9 years ago

As an aside, if you tell Node's HTTP server to .listen(0) it'll pick a random non-priveleged port to listen to, which you can then retrieve via .address().port - avoiding the error altogether. E.g.:

var http = require('http');
var server = http.createServer();
server.listen(0, function () {
  console.log('Listening on http://localhost:' + server.address().port);
})

The likely hood of it colliding on random ports is slim to none (but it can happen!)

nichoth commented 9 years ago

Nice tip

On Mon, Jun 22, 2015 at 3:31 AM, Keith Cirkel notifications@github.com wrote:

As an aside, if you tell Node's HTTP server to .listen(0) it'll pick a random non-priveleged port to listen to, which you can then retrieve via .address().port - avoiding the error altogether. E.g.:

var http = require('http');var server = http.createServer(); server.listen(0, function () { console.log('Listening on http://localhost:' + server.address().port); })

— Reply to this email directly or view it on GitHub https://github.com/keithamus/parallelshell/issues/22#issuecomment-114060679 .

pklingem commented 9 years ago

@keithamus thanks for the reply. I don't think this solves my problem though. In my case I have three tasks that parallelshell executes, a js build task, a css build task and a serve task. The css build task is the one that seems to have the most problem being killed, it uses compass. Now that I say that, maybe compass is the problem. I'll do some more digging.

keithamus commented 9 years ago

This module is supposed to exit all children on the first non-zero exit code from any child - however it seems that managing child processes is a much trickier business than I imagined. In these cases I'd ask you have a rustle around and see if you can find out whats happening, and I'll try to take a stab at fixing it so this module works as it is supposed to.

paulpflug commented 9 years ago

@keithamus are you aware, that you didn't bump the version on npm since #14?

keithamus commented 9 years ago

Ahhh, nope, I didn't realise that. I've released 1.2.0 @paulpflug. Thanks for spotting that!

keithamus commented 9 years ago

@pklingem want to try 1.2.0 and see if the error you have is still occurring?

pklingem commented 9 years ago

Thanks for following up, will give it a try and report back.

On Sun, Jun 28, 2015, 10:47 AM Keith Cirkel notifications@github.com wrote:

@pklingem https://github.com/pklingem want to try 1.2.0 and see if the error you have is still occurring?

— Reply to this email directly or view it on GitHub https://github.com/keithamus/parallelshell/issues/22#issuecomment-116287331 .

dkhuntrods commented 9 years ago

First want to say thanks for this module and your posts detailing its raison d'etre. It's becoming an essential part of my workflow. I was having a similar problem under similar circumstances and the update to 1.2.0 seems to have fixed it.

keithamus commented 9 years ago

Great :smile:. If @nichoth and @pklingem can confirm that 1.2.0 is working for them, I'll close this issue and @paulpflug is owed a huge high five :hand:

nichoth commented 9 years ago

I'm still having the same issue with version 1.2.0 using test repo https://github.com/nichoth/para-test & npm version 2.12.0.

paulpflug commented 9 years ago

looks like it is a problem of npm.. If I replace npm run watch with the content of the watch task it closes fine.

I could workaround by using node-posix and kill the process group from shell spawn(sh,[shFlag,'kill -TERM -'+posix.getpgid(0)])

keithamus commented 9 years ago

Tasks like watch/watchify should be killing their sub-processes on SIGINT, we should investigate problems there further before drastic measures like adding (native!) submodules, IMO.

If I ^C the watch task manually, it closes fine without bugging out - my understanding is that Bash simply SIGINTs the focused process' PID, so that should be enough for us to do - right? Perhaps because we're wrapping it in multiple shells, something gets lost? parallelshell "npm run dev" where dev is watch file.js "npm run build" will spin up something in the order of 4 levels deep across 8 processes (sh for parallelshell, sh for npm run dev, sh for watch, sh for npm run build). It's no wonder this isn't an open-and-shut case :sweat:.

paulpflug commented 9 years ago

I think the process hierarchy is:

npm run dev (1)
  parallelshell (2)
    sh (3)
      npm run watch (4)
        watchify (5)

But even when I eliminate (1) or (4) or both it doesn't work.

What I wrote above (If I replace npm run watch with the content of the watch task it closes fine.) doesn't work for me anymore, I think it didn't find watchify before and both child tasks errored :smiley:

Seems sh/watchify combination is troublesome. I don't understand why sending "SIGTERM" or "^C" in console behave different..

I figured out another workaround on unix systems.. If I spawn sh with detached: true, I can kill it with spawn(sh,[shFlag,'kill -TERM -'+children[i].pid]). Because in this case the pid equals the pgid

paulpflug commented 9 years ago

@keithamus have a look at #2098

mvindahl commented 9 years ago

We experienced this problem as well. Sometimes it even required us to restart our mac before we could do anything.

As an initial workaround, we switched to using the ttab module (https://www.npmjs.com/package/ttab), launching all child processes in a separate tab using ordinary Unix ampersands to separate the commands. All processes and child processes can then be terminated simply by closing the tab. It works, but it's Mac only, so it clutters the build scripts a bit.

Recently we discovered the shell-executor (https://github.com/royriojas/shell-executor) module which claims to work like parallelshell but without its problems. We haven't used it for any extended period of time yet so we have yet to verify if it holds water for us. Also, I haven't looked at the code to see how it does things differently, but I thought maybe @keithamus might find it interesting.

paulpflug commented 9 years ago

could you try the version on the dev branch, that would be kind.. unit tests say it works :smile:

mvindahl commented 9 years ago

Sure thing. I'll try it out on a pet project and see how it behaves over the next couple of weeks.

mvindahl commented 9 years ago

Looks very promising. I have started and stopped my build env a number of times and I've tried to force the individual processes to crash. No zombie processes spotted.

ffdead commented 8 years ago

Hey, any updates on this?

Also having problems with a node app.js process not being killed properly using v2.0.0

keithamus commented 8 years ago

@ffdead this project is currently no longer maintained, we hope to consolidate efforts with npm-run-all See https://github.com/mysticatea/npm-run-all/issues/10.

ffdead commented 8 years ago

Oh I see, thanks for the quick reply @keithamus !