Add stop_signal feature #601
Conversation
| if 'statspid' in props and not 'pid' in props: | ||
| raise ArgumentError('cannot specify childpid without pid') | ||
| if 'children' in props and not 'pid' in props: | ||
| raise ArgumentError('cannot specify children without pid') | ||
|
|
||
| if isinstance(signum, int): |
There was a problem hiding this comment.
I am wondering if we'd better raise when we get a SIGWINCH.
There was a problem hiding this comment.
Why SIGWINCH in particular?
We could have a blacklist if that made sense. Probably no legitimate reason to send SIGFPE, for instance. Then again, if you really want to, who am I to judge?
There was a problem hiding this comment.
Why SIGWINCH in particular?
it may happen when you resize your terminal window. So that's something we should not care about.
We could have a blacklist if that made sense. Probably no legitimate reason to send SIGFPE, for instance. Then again, if you really want to, who am I to judge?
I don't know. I am just raising the point that we currently have a whitelist and going for a free list. But maybe that's not really a problem and up to the end user. I am myself never using other signals than term & kill so I am not very well positioned to judge either
There was a problem hiding this comment.
http://nginx.org/en/docs/control.html
Nginx uses SIGWINCH to trigger a graceful shutdown of worker processes. Pretty questionable on the part of Nginx, but good reason for us to leave this wide open.
There was a problem hiding this comment.
ok thanks for the feedback on this. Notice that circusd currently intercepts it to ignore it - https://github.com/mozilla-services/circus/blob/master/circus/sighandler.py#L69
There was a problem hiding this comment.
Hmm, maybe that should come out... Any idea why that signal was specifically captured?
There was a problem hiding this comment.
IIRC that's because your app needs to handle SIGWINCH (even to ignore it) to avoid it to crash at the system level when the window is resized. The same code exists in gunicorn - it was introduced by gunicorn author when he contributed to circus in the early days.
There was a problem hiding this comment.
Oh, nasty issue. Can't believe I haven't run into that.
So does that mean that we currently have no way to actually send SIGWINCH to Nginx?
There was a problem hiding this comment.
yes we can, via the signal command - the one that's ignored is the signal sent to the circusd process itself. The syshandlers modules is to handle signals received by circusd itself, not those passed to managed processes
There was a problem hiding this comment.
Oh, OK. Sounds good then.
|
Beside the small comment, this looks great. Thanks a lot |
|
Thanks. Going to make my transition away from supervisor much simpler. We only use SIGQUIT and SIGINT for our current processes. |
Add stop_signal feature (#594)
I added the ability to specify stop_signal for a watcher and enhanced the signal command to allow any signal listed in signals.py. I moved the signal name-to-number translation from sendsignal.py to utils.