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

Enable bare except checking #45389

Open
wants to merge 19 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@abadger
Member

abadger commented Sep 8, 2018

SUMMARY
except:

if generally a bad thing to do in Python. It catches unexpected things like sys.exit() and KeyboardInterrupt. Catching Exception except Exception: is usually considered bad style but at least it doesn't have the sys.exit and keyboardInterrupt problem so switch all occurrences of bare except catching to except Exception:. Also enable the pylint test for this so that we don't regress.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

Various

ANSIBLE VERSION
2.8
ADDITIONAL INFORMATION

At the moment, each individual site has not been reviewed to see if there is a reason to catch sys.exit and KeyBoardInterrupt. I would like to get feedback from the other devs about areas which do not need review (I believe that modules, for instance, should never be catching those. And, in fact, if they are, then there's very likely a bug in the code since AnsibleModule.exit_json and AnsibleModule.fail_json call sys.exit. If there's an bare exception handler around code which calls fail_json fail_json won't trigger correctly) Once we know which areas need review, I can review those pieces to make sure we don't need to explicitly catch some other exceptions in the port.

This should go in at the beginning of the 2.8 development cycle. So within the next few weeks.

/cc @mattclay @nitzmahone @sivel

@webknjaz

I like this change.

@openstack-zuul

This comment has been minimized.

openstack-zuul bot commented Sep 8, 2018

Build succeeded (third-party-check pipeline).

@abadger abadger force-pushed the abadger:enable-bare-except-checking branch Sep 8, 2018

@openstack-zuul

This comment has been minimized.

openstack-zuul bot commented Sep 8, 2018

Build succeeded (third-party-check pipeline).

@openstack-zuul

This comment has been minimized.

openstack-zuul bot commented Sep 10, 2018

Build succeeded (third-party-check pipeline).

@ansibot ansibot added the ci_verified label Sep 10, 2018

@bcoca bcoca removed the needs_triage label Sep 10, 2018

@ansibot ansibot removed the ci_verified label Sep 10, 2018

@abadger abadger force-pushed the abadger:enable-bare-except-checking branch Sep 10, 2018

@ansibot ansibot added needs_ci and removed needs_ci labels Sep 10, 2018

@abadger

This comment has been minimized.

Member

abadger commented Sep 10, 2018

bot_skip

@openstack-zuul

This comment has been minimized.

openstack-zuul bot commented Sep 10, 2018

Build succeeded (third-party-check pipeline).

@abadger

This comment has been minimized.

Member

abadger commented Sep 10, 2018

Risk reward analysis:

Controller side:

  • Risk that we want to catch KeyboardInterrupt (for instance when processing user input) and the change would no longer do that. (Explicitly catch KeyboardInterrupt instead of/in addition to catching Exception to fix that).
  • Risk that we want to catch sys.exit (probably only if a badly written library that we depend on calls it) and (Explicitly catch SystemExit instead of/in addition to Exception to fix that).
  • Reward: we won't be catching KeyboardInterrupt unintentionally. Catching KeyboardInterrupt might be a user frustration if they're hitting Ctrl-C and nothing is happening. However, we might have some of that frustration anyways due to our multi-worker architecture.

Module side:

  • Risk that we want to catch sys.exit (probably only if badly written library that we depend on calls it).
  • Reward: modules won't inadvertantly catch SystemExit from calling module.fail_json() further down in the call stack, which would end up doing further processing after a fail_json() call.

(KeyboardInterrupt shouldn't be either a pro or con module side as there shouldn't be a way to generate that module-side).

abadger added some commits Sep 8, 2018

Update test harness code for bare-except
For thread.py, use BaseException to show we mean to catch *everything*
Ensure that current uses of BaseException are required
* In some cases, it appears that Exception should have been used instead
  as there's no need to catch sys.exit KeyboardInterrupt and similar.
* In a few cases, it appears that BaseException is used because
  a library we depend on calls sys.exit() contrary to good coding
  design.  Comment those so that we know that those have been audited
  and found to be correct.

@abadger abadger force-pushed the abadger:enable-bare-except-checking branch to f975052 Sep 12, 2018

@openstack-zuul

This comment has been minimized.

openstack-zuul bot commented Sep 12, 2018

Build succeeded (third-party-check pipeline).

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 16, 2018

@abadger How do you want to move this forward? It might be easier to enable the rule, add the necessary per-file skip entries and then submit separate PRs to fix the issues. Splitting up the fixes into multiple PRs would allow them to be grouped as needed to facilitate reviews by the appropriate people and keep slower reviews from holding up fixes for ones we're able to expedite.

@abadger

This comment has been minimized.

Member

abadger commented Oct 16, 2018

I'd like to figure out the subset of this that can be fixed without input from other people because it's deemed safe or because it is code under the core teams responsibility anyway. Then fixing those and implementing the ship for only those files that remain. Leaving it up to other people to make changes won't make quick progress so I'm loathe to just leave that.

Module side we probably should fix everything that's using bare except because it's likely that as we revamp basic.py, bare except will end up catching sys.exit/SystemExit in now instances... Fail_json won't be limited to places that have access to a module so it will become even more of a trip hazard than it currently is

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