Use of finally/return considered harmful

[Johannes Erdfelt]

Doing a return from a finally block will end up silently dropping exceptions.

This can cause unexpected behavior at runtime where unhandled exceptions are silently dropped when not intended.

This has caused some tests that would should fail because of API changes, to end up passing.

Examples are test_init_instance_stuck_in_deleting and test_init_instance_deletes_error_deleting_instance in nova/tests/compute/test_compute_mgr.py. The _delete_instance method that is being mocked out has changed, but the finally/return in nova/compute/manager.py:_init_instance has ended up ignoring the test failures causing the tests to continue passing.

[Andrew Laski (alaski)]

I don't know if hacking checks can encompass multiple line statements of this nature, but if so it would be nice to have a rule to check for this.

[Johannes Erdfelt (johannes.erdfelt)]

I'm not sure why my patch never got linked here by the gerrit bot:

https://review.openstack.org/#/c/96293/

I looked into a hacking check when I wrote the patch. pep8 allows you to get the current logical line and the previous logical line. That would be sufficient to catch a subset of cases (where return is the first statement after finally). That seemed like a poor solution.

There is also the 'lines' argument that a check can use, but it effectively requires writing a parser for the Python language.

I've thrown around the idea of parsing the AST but it was sufficiently different that I didn't know how something like that could get merged and I didn't want to hold up this patch.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/96293
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=a7afc3a04b45d75d7d73b92bfa3192631d87c50a
Submitter: Jenkins
Branch: master

commit a7afc3a04b45d75d7d73b92bfa3192631d87c50a
Author: Johannes Erdfelt <email address hidden>
Date: Wed May 28 22:11:31 2014 -0700

    Don't return from a finally block

    The code in the finally block always executes, even if an exception is
    raised. If a return is done, that will cause exceptions to be silently
    ignored.

    Some unit tests have been failing because of API changes to the
    nova.compute.manager:ComputeManager._delete_instance method but the
    use of finally/return ended up silently ignoring that error and
    caused the test to appear as if it was passing. Another unit test
    fails because of a missing uuid attribute.

    It could also potentially cause other exceptions to be ignored at
    runtime (such as SystemExit) which is likely not the intended
    behavior.

    Change-Id: I083ee7215296e35a0027be5b3ddf8b764f44cc5b
    Closes-bug: 1324277