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

Clean up child process handling #2999

Closed
10 tasks done
slackpad opened this issue May 3, 2017 · 3 comments
Closed
10 tasks done

Clean up child process handling #2999

slackpad opened this issue May 3, 2017 · 3 comments
Assignees
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/bug Feature does not function as expected
Milestone

Comments

@slackpad
Copy link
Contributor

slackpad commented May 3, 2017

This issue consolidates several similar issues with regards to child process handling. There are several areas in Consul where we spawn child processes but we have various issues:

  • consul exec
  • consul lock
  • consul watch
  • watch scripts
  • health check scripts

There are cases where we don't properly clean up and can leak child processes, or we don't handle signals properly. Some places we assume a shell, and others we don't. We should factor child process handling to be done in a uniform way (look at https://github.com/hashicorp/consul-template/tree/master/child, and maybe use that) and make sure that:

@maxenglander
Copy link

@slackpad not sure if the following issue is covered by this master ticket, but something I've observed is that consul watch doesn't return the exit code of it's handler. E.g.

$ consul watch -prefix foo/bar sh -c 'exit 2'; echo $?
Error executing handler: exit status 2
1

This makes it difficult to integrate consul watch into a supervisor process that takes certain actions or not (e.g. restart or not) based on exit codes.

Let me know if it's not covered, I'll happily open a new ticket.

@slackpad
Copy link
Contributor Author

slackpad commented Oct 3, 2017

Hi @maxenglander that's not captured here and is probably better on a separate issue. A similar change for consul lock was done under #947.

@maxenglander
Copy link

Aha! Thanks @slackpad, opened #3526.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/bug Feature does not function as expected
Projects
None yet
Development

No branches or pull requests

4 participants