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 signal command and stopping procedure #603

Merged
merged 13 commits into from
Oct 26, 2013
Merged

Fix signal command and stopping procedure #603

merged 13 commits into from
Oct 26, 2013

Conversation

scottkmaxwell
Copy link
Contributor

The signal command was fairly broken, especially from the command line. Sending a signal to all processes of a watcher call watcher.send_signal_processes(), which does not exist. The recursive and children flags defaulted to true, contrary to the docs. The command-line took an unused 'process' parameter. And children and recursive flags were only used with the 3 argument version of the command-line.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling debdcab on scottkmaxwell:fix-signal-command into 613c892 on mozilla-services:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b6f656b on scottkmaxwell:fix-signal-command into 613c892 on mozilla-services:master.

@scottkmaxwell
Copy link
Contributor Author

Hmm, going to make sending stop_signal to the children optional... Please hold.

…ends to parent first and then all children

Previously, if a single child was already dead, the code would have given up
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1248513 on scottkmaxwell:fix-signal-command into 613c892 on mozilla-services:master.

@scottkmaxwell
Copy link
Contributor Author

Done. Previous 'kill' logic had a pretty serious flaw. It iterated through the children first, sending each the stop command, then killed the parent. If any process was already dead, the entire method returned. So if a single child had already died, the kill would never have gotten sent to the parent.

Also, 2 more points. For many apps, you do not want to send the initial stop signal to the children. The parent is expected to cleanly shut down its children when it gets the stop command. So I added a 'stop_children' parameter that defaults to false. Secondly, when we have to issue a SIGKILL, we need to kill the parent first so that it doesn't start respawning children.

@scottkmaxwell
Copy link
Contributor Author

I understand that changing kill logic is a bit risky, but I'm pretty certain that this version is more correct. However, it is possible that people have circus in production today with apps that expect the kill signal to be sent to all children. Those children would still get shutdown since the SIGKILL always does children, but they would not have the opportunity to shutdown gracefully. So an argument could be made for having 'stop_children' default to true. But if this is not a large concern, I think defaulting to false is usually more correct.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f53fa3f on scottkmaxwell:fix-signal-command into 613c892 on mozilla-services:master.

@tarekziade
Copy link
Member

Sending a signal to all processes of a watcher call watcher.send_signal_processes(), which does not exis

Ouch :/ - I suspect we need to make sure this is covered by a test.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2187dee on scottkmaxwell:fix-signal-command into c83a4d9 on mozilla-services:master.

@scottkmaxwell
Copy link
Contributor Author

Need to fix Py3. :-) Just a minute.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2abcd17 on scottkmaxwell:fix-signal-command into c83a4d9 on mozilla-services:master.

…t variable isn't explicitly set. Travis needs more time.
@scottkmaxwell
Copy link
Contributor Author

Travis needs more time...

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7a8f829 on scottkmaxwell:fix-signal-command into c83a4d9 on mozilla-services:master.

@scottkmaxwell
Copy link
Contributor Author

OK, changing default to 10 seconds for ASYNC_TEST_TIMEOUT allowed a clean run through Travis. This should be good to go.

@tarekziade
Copy link
Member

Great job thanks a lot

tarekziade added a commit that referenced this pull request Oct 26, 2013
Fix signal command and stopping procedure
@tarekziade tarekziade merged commit fb43aba into circus-tent:master Oct 26, 2013
@scottkmaxwell scottkmaxwell deleted the fix-signal-command branch October 30, 2014 21:19
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

3 participants