fix for #22

[paulpflug]

works for me in linux

needs to be tested on unix and windows

[paulpflug]

sometimes I hate git >_<

[paulpflug]

I have now tested it on windows. Works as expected 😄

[andrenarchy]

Is there any reason why this can't be merged?

[keithamus]

I'm a little dubious to merge - it feels a bit like a side effect. I wanted to do some proper research on this before deciding on what to do - which I realise now has been nearly a month, so not very fair to @paulpflug.

The nodejs/node#2098 doesn't fill me with confidence about this patch. I'm not sure which way to go with it. Advice would be helpful 😄

[paulpflug]

I can fully understand that..

The underlying problem is, that sh decides to ignore kill orders and the node people say thats the way it is and should be ;)
So there are two options:

1. Go away from sh
2. Kill it with the process group ID (PGID) (which has to be obtained somehow)

[andrenarchy]

The problem is that sh (which is dash on my Ubuntu system!) spawns a new process for the provided command, so sh -c "node ..." results in the process hierarchy

+ parent
  + sh
    + node ...

Since dash (like bash) does not forward signals to children by default, the node process will still run after killing sh. The node process is reparented.

Note: if bash is used, the command replaces the current shell and the process hierarchy is

+parent
  + node ...

Then, killing the bash process will kill node.

So option 3 is: invoke processes with sh -c "exec node ..." instead of sh -c "node ...". Then, the sh process is replaced (see also here) and all kills should work as expected.

What do you think?

[paulpflug]

@andrenarchy that was very helpful, I implemented the exec way.. very clean, thank you 😄
@keithamus in my eyes it can be merged now

[keithamus]

👍

[keithamus]

Glad we got to the bottom of this 😄

[keithamus]

@paulpflug you've got push access on here, and pub access on npm now. Feel free to add improvements as you see fit, but please avoid committing directly to master (unless its a release commit) - keep stuff to PRs for visibility.

[paulpflug]

sounds reasonable 😄
thanks

[andrenarchy]

Unfortunately, using exec breaks chained commands. 😦

For example, running parallelshell "echo test1 && echo test2" only prints test1. The shell obviously is replaced with the first command.

[paulpflug]

We could catch && and handle it like parallelshell "echo test1" "echo test", what do you think?

[andrenarchy]

Probably not a good idea. You'd have to catch all kinds of shell commands (||, ;) and do variable substitution. In the end, you'd write a full shell. ;)

I'm also investigating if there's another way...

[estk]

Yeah this broke command chaining.
There is a workaround which I think re-breaks #22:
parallelshell "sh -c 'echo first && echo second'" "echo third"

[keithamus]

@estk could you try testing out #30 and see if the code in that PR works for your use cases?