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

fix process termination when /bin/sh is BusyBox #41

Merged
merged 1 commit into from Jun 16, 2017
Merged

fix process termination when /bin/sh is BusyBox #41

merged 1 commit into from Jun 16, 2017

Conversation

neelance
Copy link
Contributor

It seems like bash is using exec by default for single commands provided via -c. BusyBox however is not. If /bin/sh is BusyBox (e.g. on Docker images based on Alpine) then the specified command becomes a subprocess of /bin/sh. This messes up signal handling and goreman fails to terminate the subprocesses when receiving a SIGTERM. Adding an explicit exec should fix this without modifying any command parsing.

It seems like bash is using `exec` by default for single commands provided via `-c`. BusyBox however is not. If `/bin/sh` is BusyBox (e.g. on Docker images based on Alpine) then the specified command becomes a subprocess of `/bin/sh`. This messes up signal handling and goreman fails to terminate the subprocesses when receiving a SIGTERM. Adding an explicit `exec` should fix this without modifying any command parsing.
@neelance
Copy link
Contributor Author

This may address the same as #30.

@mattn
Copy link
Owner

mattn commented Jun 16, 2017

Right. LGTM

@mattn mattn merged commit d0ee41b into mattn:master Jun 16, 2017
@neelance
Copy link
Contributor Author

Thanks for the quick merge!

@keegancsmith
Copy link

FYI previously goreman worked if your command started with environment specifiers, or if you used more complicated shell. eg: both these used to work

envExample: FOO=bar mycommand
shellExample: sleep 10 && echo hi

goreman now fails on both of those. The first failure is somewhat obvious since goreman will complain that it can't find the command FOO=bar. However, the second is subtle since exec sleep 10 && echo hi will exec sleep 10 and therefore the && echo hi is never evaluated. So you just get a silent failure.

some documentation or shell parsing would be useful in this case. I just updated my goreman, hence me only noticing this now.

@mattn
Copy link
Owner

mattn commented Dec 5, 2017

Hmm, sorry I didnot consider it.

@mattn
Copy link
Owner

mattn commented Dec 5, 2017

@neelance do you know the way to pass command-line into the busybox shell? and way to send signal to the process group?

@neelance
Copy link
Contributor Author

Is there some standard on how foreman tools are supposed to behave, e.g. which shell features are supposed to be supported?

The first case that @keegancsmith mentioned could be solved by prepending env, but the second can't. I also just realized that in the case of the issue I had, it might have been possible to add the exec to the Procfile itself.

I'm not sure what the best thing to do here is. But I think it would be better to avoid behavior that is different depending on which sh your system has.

@keegancsmith
Copy link

There doesn't really seem to be a standard. One could check the most popular Procfile runner and see how it behaves. Fixing signal behaviour is the correct thing to do, and prefixing exec seems like a good solution. From reading the POSIX standard I don't see anything preventing or condoning the automatic exec bash does. The only real way around this is to write a little wrapper script that traps the signal and manually sends it, which doesn't seem like a great thing.

I think the best thing would be to somehow detect when there is a compound command, and Goreman warns/exits with a good error message to the user.

@mattn
Copy link
Owner

mattn commented Dec 22, 2017

How about to support both with new flag or environment variable?

@mattn
Copy link
Owner

mattn commented Mar 18, 2018

I'll revert this change.

mattn added a commit that referenced this pull request Mar 18, 2018
mattn added a commit that referenced this pull request Mar 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants