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

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

Open
nichoth opened this issue May 26, 2015 · 24 comments
Open

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

nichoth opened this issue May 26, 2015 · 24 comments

Comments

@nichoth
Copy link
Contributor

nichoth commented May 26, 2015

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
Copy link

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

@nichoth
Copy link
Contributor Author

nichoth commented Jun 21, 2015

It happens rarely enough I haven't tried fixing.

@keithamus
Copy link
Collaborator

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
Copy link
Contributor Author

nichoth commented Jun 22, 2015

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
#22 (comment)
.

@pklingem
Copy link

@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
Copy link
Collaborator

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
Copy link
Collaborator

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

@keithamus
Copy link
Collaborator

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

@keithamus
Copy link
Collaborator

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

@pklingem
Copy link

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
#22 (comment)
.

@dkhuntrods
Copy link

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
Copy link
Collaborator

Great 😄. 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 ✋

@nichoth
Copy link
Contributor Author

nichoth commented Jul 2, 2015

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
Copy link
Collaborator

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
Copy link
Collaborator

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 😓.

@paulpflug
Copy link
Collaborator

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 😃

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 added a commit to paulpflug/parallelshell that referenced this issue Jul 2, 2015
works for me in linux

needs to be tested on unix and windows
@paulpflug
Copy link
Collaborator

@keithamus have a look at #2098

@mvindahl
Copy link

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.

keithamus added a commit that referenced this issue Jul 28, 2015
@paulpflug
Copy link
Collaborator

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

@mvindahl
Copy link

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

@mvindahl
Copy link

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.

@estk estk mentioned this issue Sep 19, 2015
estk pushed a commit to estk/parallelshell that referenced this issue Sep 21, 2015
works for me in linux

needs to be tested on unix and windows
estk pushed a commit to estk/parallelshell that referenced this issue Sep 21, 2015
@ffdead
Copy link

ffdead commented Apr 5, 2016

Hey, any updates on this?

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

@keithamus
Copy link
Collaborator

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

@ffdead
Copy link

ffdead commented Apr 5, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants