Project

Profile

Help

Issue #341

closed

Eliminate return statements from try, finally blocks

Added by rbarlow about 9 years ago. Updated almost 5 years ago.

Status:
CLOSED - WONTFIX
Priority:
Normal
Assignee:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Master
Platform Release:
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
Sprint:
Quarter:

Description

I learned something interesting about Python exception handling today that may be a problem in our code. If a try, finally block has a return statement in the finally section, the return statement will squash any Exception that may have been raised.

I haven't done any analysis on our codebase at all, except that I have seen this in one place inside our Puppet distributor's publish handling code. In the case of that particular code, we might want a try, except block instead of a try, finally anyway.

For the purposes of this ticket, I think we should do an audit of our Exception handling in pulp, looking for return statements in finally blocks. There may be no further violations than what I've found so far, but I'd like us to make sure. In each case that we find, we need to decide what is the most appropriate way to proceed. In some cases, we can just bring the return statement outside the try block, or in other cases we might be able to convert a try, finally into a try, except.

For concreteness, this code will not raise an Exception:

try:
raise Exception("This will never see the light of day!")
finally:
clean_up_things()
return "Everything is fine, nothing is ruined."

In fact, that code will simply return that everything is fine, a blatant lie!

It would be better to do this:

try:
raise Exception("Everything is ruined, nothing is fine!")
finally:
clean_up_things()
return "Everything is fine, nothing is ruined."

With that code, we will clean up things and raise the Exception instead of returning that everything is fine. In some cases, like in the case of the publish handler for Puppet's distributor, I think we can just use a try, except pattern instead of try, finally.

jdob and I talked, and we agree that this ticket should be pushed back until after we release 2.0.

+ This bug was cloned from Bugzilla Bug #876754 +

Actions #1

Updated by cduryee about 9 years ago

I took a quick look at platform and rpm, neither do this.

Reassigning BZ to puppet component since that's the only one I could find that had returns in finally blocks.

+ This comment was cloned from Bugzilla #876754 comment 1 +

Actions #2

Updated by rbarlow about 9 years ago

This is actually a defect if it is happening, so we shouldn't mark it as a task.

+ This comment was cloned from Bugzilla #876754 comment 2 +

Actions #3

Updated by bmbouter about 9 years ago

  • Severity changed from Medium to 2. Medium
Actions #4

Updated by bmbouter almost 5 years ago

  • Status changed from NEW to CLOSED - WONTFIX

Pulp 2 is approaching maintenance mode, and this Pulp 2 ticket is not being actively worked on. As such, it is being closed as WONTFIX. Pulp 2 is still accepting contributions though, so if you want to contribute a fix for this ticket, please reopen or comment on it. If you don't have permissions to reopen this ticket, or you want to discuss an issue, please reach out via the developer mailing list.

Actions #5

Updated by bmbouter almost 5 years ago

  • Tags Pulp 2 added

Also available in: Atom PDF