launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/launchpad/trivial into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #902063 in Launchpad itself: "LoopTuner loses  exceptions if cleanUp fails"
  https://bugs.launchpad.net/launchpad/+bug/902063

For more details, see:
https://code.launchpad.net/~stub/launchpad/trivial/+merge/85126

If a LoopTuner run terminated due to an unhandled exception, that exception would be lost if the cleanup routine also raised an exception. This is quite common, for instance when the unhandled exception is a database exception causing the database connection to go into an unusable state. So don't do that.
-- 
https://code.launchpad.net/~stub/launchpad/trivial/+merge/85126
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/trivial into lp:launchpad.

=== modified file 'lib/canonical/launchpad/utilities/looptuner.py'
--- lib/canonical/launchpad/utilities/looptuner.py	2011-09-19 06:57:55 +0000
+++ lib/canonical/launchpad/utilities/looptuner.py	2011-12-09 13:17:44 +0000
@@ -118,6 +118,8 @@
 
     def run(self):
         """Run the loop to completion."""
+        # Cleanup function, if we have one.
+        cleanup = getattr(self.operation, 'cleanUp', lambda: None)
         try:
             chunk_size = self.minimum_chunk_size
             iteration = 0
@@ -169,9 +171,20 @@
                 "average size %f (%s/s)",
                 total_size, iteration, total_time, average_size,
                 average_speed)
-        finally:
-            if safe_hasattr(self.operation, 'cleanUp'):
-                self.operation.cleanUp()
+        except Exception:
+            # Should we be logging this? Raising an exception might be a
+            # way of escaping the loop early, although making isDone
+            # signal termination is probably better.
+            self.log.exception("Unhandled exception")
+            try:
+                cleanup()
+            except Exception:
+                self.log.exception("Unhandled exception in cleanUp")
+            raise
+        try:
+            cleanup()
+        except Exception:
+            self.log.exception("Unhandled exception in cleanUp")
 
     def _coolDown(self, bedtime):
         """Sleep for `self.cooldown_time` seconds, if set.