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

Don't silently catch SystemExit #49241

Merged
merged 2 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@terminalmage
Member

terminalmage commented Aug 21, 2018

In integration.shell.test_syndic.SyndicTest.test_issue_7754, we use run_script to start up a syndic and make sure that it starts with specific logging configuration. run_script is configured to timeout after 5 seconds, at which time the process is killed via SIGTERM. Our signal handler then raises a SystemExit to kill the Python process. However, if this SystemExit is raised while _connect_syndic() is attempting to connect to the master-of-masters, it gets swallowed and the Python process never exits, causing the test suite to hang indefinitely (until test-kitchen kills it).

This PR ensures we re-raise a SystemExit if it is caught during this connection attempt.

In addition, it removes the bare except that was being used in this same try block. Friends don't let friends use bare excepts.

Refs: saltstack/salt-jenkins#1078

terminalmage added some commits Aug 21, 2018

Don't use a bare except!
Every time you use a bare except, Cthulu kills a kitten. Please, think
of the kittens.
Don't silently catch SystemExit
In `integration.shell.test_syndic.SyndicTest.test_issue_7754`, we use
`run_script` to start up a syndic and make sure that it starts with
specific logging configuration. `run_script` is configured to timeout
after 5 seconds, at which time the process is killed via SIGTERM. Our
signal handler then raises a `SystemExit` to kill the Python process.
However, if this `SystemExit` is raised while `_connect_syndic()` is
attempting to connect to the master-of-masters, it gets swallowed and
the Python process never exits, causing the test suite to hang
indefinitely (until test-kitchen kills it).

This commit ensures we re-raise a `SystemExit` if it is caught during
this connection attempt.

@terminalmage terminalmage requested a review from saltstack/team-core as a code owner Aug 21, 2018

@terminalmage terminalmage requested a review from Ch3LL Aug 21, 2018

@DmitryKuzmenko

This comment has been minimized.

Contributor

DmitryKuzmenko commented Aug 22, 2018

@terminalmage BTW:

$ grep -Rn "^ *except:" salt/ | wc -l
48

@gtmanfred gtmanfred merged commit bc0b4ac into saltstack:2018.3.3 Aug 22, 2018

7 of 8 checks passed

jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@terminalmage terminalmage deleted the terminalmage:salt-jenkins-1078 branch Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment