Method

The analysis is based on the 8,000 most popular PyPI packages, in terms of numberof downloads in the last 30 days. They were downloaded on the 17th-18th ofOctober, usinga scriptwritten by Guido van Rossum, which in turn relies on Hugo van Kemenade’stool that creates a list of themost popular packages.

Once downloaded, asecond scriptwas used to construct an AST for each file, and traverse it to identifybreak,continue andreturn statements which are directly inside afinally block.

I then found the current source code for each occurrence, and categorized it. Forcases where the code seems incorrect, I created an issue in the project’s bugtracker. The responses to these issues are also part of the data collected inthis investigation.

Results

I decided not to include a list of the incorrect usages, out of concern thatit would make this report look like a shaming exercise. Instead I will describethe results in general terms, but will mention that some of the problems I foundappear in very popular libraries, including a cloud security application.For those so inclined, it should not be hard to replicate my analysis, as Iprovided links to the scripts I used in the Method section.

The projects examined contained a total of 120,964,221 lines of Python code,and among them the script found 203 instances of control flow instructions in afinally block. Most werereturn, a handful werebreak, and none werecontinue. Of these:

• 46 are correct, and appear in tests that target this pattern as a feature (e.g.,tests for linters that detect it).
• 8 seem like they could be correct - either intentionally swallowing exceptionsor appearing where an active exception cannot occur. Despite being correct, it isnot hard to rewrite them to avoid the bad pattern, and it would make the codeclearer: deliberately swallowing exceptions can be more explicitly done withexceptBaseException:, andreturn which doesn’t swallow exceptions can bemoved after thefinally block.
• 149 were clearly incorrect, and can lead to unintended swallowing of exceptions.These are analyzed in the next section.

The Error Cases

Many of the error cases followed this pattern:

try:...exceptSomeSpecificError:...exceptException:logger.log(...)finally:returnsome_value

Code like this is obviously incorrect because it deliberately logs and swallowsException subclasses, while silently swallowingBaseExceptions. The intentionhere is either to allowBaseExceptions to propagate on, or (if the author isunaware of theBaseException issue), to log and swallow all exceptions. However,even if theexceptException was changed toexceptBaseException, this codewould still have the problem that thefinally block swallows all exceptionsraised from within theexcept block, and this is probably not the intention(if it is, that can be made explicit with anothertry-exceptBaseException).

Another variation on the issue found in real code looks like this:

try:...except:returnNotImplementedfinally:returnsome_value

Here the intention seems to be to returnNotImplemented when an exception israised, but thereturn in thefinally block would override the one in theexcept block.

Author reactions

Of the 149 incorrect instances ofreturn orbreak in afinally clause,27 were out of date, in the sense that they do not appear in the main/master branchof the library, as the code has been deleted or fixed by now. The remaining 122are in 73 different packages, and I created an issue in each one to alert theauthors to the problems. Within two weeks, 40 of the 73 issues received a reactionfrom the code maintainers:

• 15 issues had a PR opened to fix the problem.
• 20 received reactions acknowledging the problem as one worth looking into.
• 3 replied that the code is no longer maintained so this won’t be fixed.
• 2 closed the issue as “works as intended”, one said that they intend toswallow all exceptions, but the other seemed unaware of the distinctionbetweenException andBaseException.

One issue was linked to a pre-existing open issue about non-responsiveness to Ctrl-C,conjecturing a connection.

Two of the issue were labelled as “good first issue”.

The correct usages

The 8 cases where the feature appears to be used correctly (in non-test code) alsodeserve attention. These represent the “churn” that would be caused by blockingthe feature, because this is where working code will need to change. I did notcontact the authors in these cases, so we need to assess the difficulty ofmaking these changes ourselves. It is shown inthe full report,that the change required in each case is small.

Discussion

The first thing to note is thatreturn/break/continue in afinallyblock is not something we see often: 203 instance in over 120 million linesof code. This is, possibly, thanks to the linters that warn about this.

The second observation is that most of the usages were incorrect: 73% in oursample (149 of 203).

Finally, the author responses were overwhelmingly positive. Of the 40 responsesreceived within two weeks, 35 acknowledged the issue, 15 of which also createda PR to fix it. Only two thought that the code is fine as it is, and threestated that the code is no longer maintained so they will not look into it.

The 8 instances where the code seems to work as intended, are not hard torewrite.