launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-994694 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #994694 in Launchpad itself: "GeneratorExit exception throughout paralleltest subunit output"
  https://bugs.launchpad.net/launchpad/+bug/994694

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-994694/+merge/105090

Bug 994694 describes some odd output resulting from a generator not
reacting correctly to being closed before yielding all of their values.
This branch fixes that bug.

The fix was simple, add GeneratorExit to the list of exceptions reraised
by get_email_notifications.  The bulk of the change is dedicated to
testing the fix.  The comments in the test describe it sufficiently (I
hope), so I won't address that here.

LOC Rationale: The entirety of the LOC increase is in tests.

Tests: The new test can be run with this command: 

    bin/test -c -t test_early_exit

Removing the GeneratorExit from the try/except in
get_email_notifications will cause the test to fail.

The "make lint" report is clean.

-- 
https://code.launchpad.net/~benji/launchpad/bug-994694/+merge/105090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-994694 into lp:launchpad.

=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2012-03-13 00:45:33 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2012-05-08 17:15:23 +0000
@@ -294,7 +294,7 @@
         # being sent, so catch and log all exceptions.
         try:
             yield construct_email_notifications(batch)
-        except (KeyboardInterrupt, SystemExit):
+        except (KeyboardInterrupt, SystemExit, GeneratorExit):
             raise
         except:
             log.exception("Error while building email notifications.")

=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2012-05-03 00:08:11 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2012-05-08 17:15:23 +0000
@@ -4,10 +4,12 @@
 
 __metaclass__ = type
 
+import StringIO
 from datetime import (
     datetime,
     timedelta,
     )
+import logging
 import re
 import unittest
 
@@ -240,12 +242,13 @@
                          'some bug task identifier:status')
 
 
-class TestGetEmailNotifications(unittest.TestCase):
+class TestGetEmailNotifications(TestCase):
     """Tests for the exception handling in get_email_notifications()."""
     layer = LaunchpadZopelessLayer
 
     def setUp(self):
         """Set up some mock bug notifications to use."""
+        super(TestGetEmailNotifications, self).setUp()
         switch_dbuser(config.malone.bugnotification_dbuser)
         sample_person = getUtility(IPersonSet).getByEmail(
             'test@xxxxxxxxxxxxx')
@@ -290,6 +293,7 @@
         sm.registerUtility(self._fake_utility)
 
     def tearDown(self):
+        super(TestGetEmailNotifications, self).tearDown()
         sm = getSiteManager()
         sm.unregisterUtility(self._fake_utility)
         sm.registerUtility(self._original_utility)
@@ -383,6 +387,44 @@
         bug_four = getUtility(IBugSet).get(4)
         self.assertEqual(bug_four.id, 4)
 
+    def test_early_exit(self):
+        # When not-yet-exhausted generators need to be deallocated Python
+        # raises a GeneratorExit exception at the point of their last yield.
+        # The get_email_notifications generator was catching that exception in
+        # a try/except and logging it, leading to bug 994694.  This test
+        # verifies that the fix for that bug (re-raising the exception) stays
+        # in place.
+
+        # Set up logging so we can later assert that no exceptions are logged.
+        log_output = StringIO.StringIO()
+        logger = logging.getLogger()
+        log_handler = logging.StreamHandler(log_output)
+        logger.addHandler(logging.StreamHandler(log_output))
+        self.addCleanup(logger.removeHandler, log_handler)
+
+        # Make some data to feed to get_email_notifications.
+        person = getUtility(IPersonSet).getByEmail('test@xxxxxxxxxxxxx')
+        msg = getUtility(IMessageSet).fromText('', '', owner=person)
+        bug = MockBug(1, person)
+        # We need more than one notification because we want the generator to
+        # stay around after being started.  Consuming the first starts it but
+        # since the second exists, the generator stays active.
+        notifications = [
+            MockBugNotification(
+                message=msg, bug=bug, is_comment=True, date_emailed=None),
+            MockBugNotification(
+                message=msg, bug=bug, is_comment=True, date_emailed=None),
+            ]
+
+        # Now we create the generator, start it, and then close it, triggering
+        # a GeneratorExit exception inside the generator.
+        email_notifications = get_email_notifications(notifications)
+        email_notifications.next()
+        email_notifications.close()
+
+        # Verify that no "Error while building email notifications." is logged.
+        self.assertEqual('', log_output.getvalue())
+
 
 class TestNotificationCommentBatches(unittest.TestCase):
     """Tests of `notification_comment_batches`."""