Page MenuHomePhabricator

Tidy up some low-level exception handling
ClosedPublic

Authored by demon on Sep 27 2017, 8:33 PM.

Details

Reviewers
mmodell
thcipriani
Group Reviewers
Release-Engineering-Team
Commits
rMSCAadbeba3d447f: Tidy up some low-level exception handling
Patch without arc
git checkout -b D795 && curl -L https://phabricator.wikimedia.org/D795?download=true | git apply
Summary
  • Use OSError with EPERM as our error condition when being run as root. SystemExit is really designed for stuff that calls sys.exit() and other low-level stuff. OSError is a far better description of what's happening here.
  • Remove weird _handle_system_exit() abstraction. I'm not even sure why we need this since none of our code outside of here calls sys.exit() or raises this exception, but at least it's slighly less confusing now

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

demon created this revision.Sep 27 2017, 8:33 PM
Restricted Application added a reviewer: mmodell. · View Herald TranscriptSep 27 2017, 8:33 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald Transcript
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
demon added inline comments.Sep 27 2017, 8:35 PM
scap/cli.py
324–325

To be perfectly honest, I'm not sure why we want to re-raise in this condition at all... there's no reason we can't handle it gracefully below and tbh nobody should be calling SystemExit anyway.

Looks like the build failure is related to unresolvable dependency - we require 1.5.x of flake8 and it seems to want to install 1.6

thcipriani accepted this revision.Sep 28 2017, 8:54 PM
thcipriani added a subscriber: thcipriani.
In D795#15869, @mmodell wrote:

Looks like the build failure is related to unresolvable dependency - we require 1.5.x of flake8 and it seems to want to install 1.6

Rebase should fix this now. Something weird happening with flake8 dependencies. It's fixed in master though.

Change looks fine to me, I think that function was built out in anticipation of future use that never materialized.

This revision is now accepted and ready to land.Sep 28 2017, 8:54 PM

longer rambling thoughts since you asked :)

scap/cli.py
309

seems more correct :)

324–325

Couple of thoughts on this:

  1. raise ensures that only the code in finally is actually executed, not the code beneath the bunch of exceptions here. Which, at the time this was written, was probably desirable since this function could return or sys.exit. Not really important to not execute the end of this function now (and probably not desirable).
  2. Perhaps (speculating) the idea was that sys.exit could be used in future if we ever wanted to skip any further handling at the end of this run function.
  3. raise is an easy way ensuring the program exits with the exit code for sys.exit which is mostly what the code at the end of this function does/did.

These are mostly speculation. Have to ask @bd808 for the straight dope.

Now I'm not entirely sure it's desirable to raise here, really since it skips logging.shutdown (or maybe we should move logging.shutdown into _before_exit). It might be better to restore _handle_system_exit in case we do ever decide to use sys.exit anywhere:

def _handle_system_exit(ex):
   return ex.message

But really I have no strong opinion on this. This seems fine.

bd808 added inline comments.Sep 29 2017, 5:36 PM
scap/cli.py
324–325

Have to ask @bd808 for the straight dope.

From a commit made Mar 2, 2014... yeah. I think the original was a very generic fatal error cascade handler probably copied from some bot wrapper I had written at some other point in my life. If you look at rMSCAbd5f470beaad: Add base class for creating CLI wrappers you will see that things were pretty basic.

demon updated this revision to Diff 2126.Oct 3 2017, 1:00 AM
  • Remove SystemExit catch entirely -- no point really per code review
demon marked 4 inline comments as done.Oct 3 2017, 1:01 AM
demon updated this revision to Diff 2127.Oct 3 2017, 1:11 AM
  • Rebased
This revision was automatically updated to reflect the committed changes.