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

Update Process signal handling #59

Merged
merged 1 commit into from
Apr 30, 2015

Conversation

jcredding
Copy link
Member

This updates the Process signal handling to follow best practices
and do as little as possible when trapping signals. It now simply
writes the signal to an IO pipe. The idea is that you want to avoid
the logic for trapping a signal raising any errors. The previous
implementation didn't do a good job of this because it would change
the daemon's state, write to an IO pipe and write to redis (by
calling stop or halt on the daemon). This left a greater chance
for it to fail while handling the signal which might cause the
system to get into a bad state. This also makes it better handle
multiple signals being sent to the process and avoid getting in a
bad state from trying to handle them all simultaneously.

This also does some minor improvements to the Process. It now
logs when it receives and handles signals. This is to make it more
clear what the process is doing. Its also more robust in that it
won't error if we start using signals that are not supported on
some operating systems.

Finally, this removes the datetime from the bench and system test
logs. The default datetime is very long and makes the logs
annoying to read.

@kellyredding - Ready for review.

This updates the `Process` signal handling to follow best practices
and do as little as possible when trapping signals. It now simply
writes the signal to an IO pipe. The idea is that you want to avoid
the logic for trapping a signal raising any errors. The previous
implementation didn't do a good job of this because it would change
the daemon's state, write to an IO pipe and write to redis (by
calling `stop` or `halt` on the daemon). This left a greater chance
for it to fail while handling the signal which might cause the
system to get into a bad state. This also makes it better handle
multiple signals being sent to the process and avoid getting in a
bad state from trying to handle them all simultaneously.

This also does some minor improvements to the `Process`. It now
logs when it receives and handles signals. This is to make it more
clear what the process is doing. Its also more robust in that it
won't error if we start using signals that are not supported on
some operating systems.

Finally, this removes the datetime from the bench and system test
logs. The default datetime is very long and makes the logs
annoying to read.
@kellyredding
Copy link
Member

@jcredding thanks for talking me through the diff. I really like the cleanups here - good stuff 👎

jcredding added a commit that referenced this pull request Apr 30, 2015
@jcredding jcredding merged commit 422ebf9 into master Apr 30, 2015
@jcredding jcredding deleted the jcr-process-better-handle-signals branch April 30, 2015 19:08
jcredding added a commit that referenced this pull request May 1, 2015
* Pull out `IOPipe`, setup for use in the `Process` (#57)
* Don't use `gets` and `puts` in `IOPipe` (#58)
* Update `Process` signal handling (#59)
* Change process name, don't allow completely changing it (#60)
* Rescue redis errors while dequeueing (#61)
* Shutdown the worker pool if an error occurs in work loop (#62)
* Use constants for halt, stop and restart signals (#63)

/cc @kellyredding
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.

2 participants