launchpad-reviewers team mailing list archive

[Raphaël Victor Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/rabbit-bug-860422 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #860422 in Launchpad itself: "RabbitUnreliableSession suppresses exceptions without logging them"
  https://bugs.launchpad.net/launchpad/+bug/860422

For more details, see:
https://code.launchpad.net/~rvb/launchpad/rabbit-bug-860422/+merge/78116

This branch fixes RabbitUnreliableSession.finish in such a way that any other error raised (i.e. not listed in suppressed_errors) will be suppressed. Also, for these errors, an OOPS is recorded.  This will isolate failures in Rabbit from the rest of LP while allowing us to diagnose any Rabbit specific problems.

= Tests =

./bin/test -vvc test_rabbit 

= Q/A =

None.
-- 
https://code.launchpad.net/~rvb/launchpad/rabbit-bug-860422/+merge/78116
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/rabbit-bug-860422 into lp:launchpad.

=== modified file 'lib/lp/services/messaging/rabbit.py'
--- lib/lp/services/messaging/rabbit.py	2011-10-03 13:42:22 +0000
+++ lib/lp/services/messaging/rabbit.py	2011-10-04 15:46:43 +0000
@@ -12,12 +12,15 @@
 from collections import deque
 from functools import partial
 import json
+import sys
 import threading
 import time
 
 from amqplib import client_0_8 as amqp
 import transaction
 from transaction._transaction import Status as TransactionStatus
+from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import implements
 
 from canonical.config import config
@@ -148,9 +151,13 @@
     silently suppressed, `AMQPException` in particular. This means that
     services can continue to function even in the absence of a running and
     fully functional message queue.
+
+    Other types of errors are also catched because we don't want this
+    subsystem to destabilise other parts of Launchpad but we nonetheless
+    record OOPses for these.
     """
 
-    ignored_errors = (
+    suppressed_errors = (
         IOError,
         MessagingException,
         amqp.AMQPException,
@@ -159,12 +166,16 @@
     def finish(self):
         """See `IMessageSession`.
 
-        Suppresses errors listed in `ignored_errors`.
+        Suppresses errors listed in `suppressed_errors`. Also suppresses
+        other errors but file an oops report for these.
         """
         try:
             super(RabbitUnreliableSession, self).finish()
-        except self.ignored_errors:
+        except self.suppressed_errors:
             pass
+        except Exception:
+            error_utility = getUtility(IErrorReportingUtility)
+            error_utility.raising(sys.exc_info())
 
 
 # Per-thread "unreliable" sessions.

=== modified file 'lib/lp/services/messaging/tests/test_rabbit.py'
--- lib/lp/services/messaging/tests/test_rabbit.py	2011-10-03 13:57:03 +0000
+++ lib/lp/services/messaging/tests/test_rabbit.py	2011-10-04 15:46:43 +0000
@@ -16,8 +16,10 @@
 from zope.component import getUtility
 from zope.event import notify
 
+from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
 from canonical.launchpad.webapp.interfaces import FinishReadOnlyRequestEvent
 from canonical.testing.layers import (
+    FunctionalLayer,
     LaunchpadFunctionalLayer,
     RabbitMQLayer,
     )
@@ -199,6 +201,22 @@
 class TestRabbitUnreliableSession(TestRabbitSession):
 
     session_factory = RabbitUnreliableSession
+    layer = FunctionalLayer
+
+    def setUp(self):
+        super(TestRabbitUnreliableSession, self).setUp()
+        self.error_utility = ErrorReportingUtility()
+        self.prev_oops = self.error_utility.getLastOopsReport()
+
+    def assertNoOops(self):
+        oops_report = self.error_utility.getLastOopsReport()
+        self.assertEqual(repr(self.prev_oops), repr(oops_report))
+
+    def assertOops(self, text_in_oops):
+        oops_report = self.error_utility.getLastOopsReport()
+        self.assertNotEqual(
+            repr(self.prev_oops), repr(oops_report), 'No OOPS reported!')
+        self.assertTrue(text_in_oops in str(oops_report))
 
     def raise_AMQPException(self):
         raise amqp.AMQPException(123, "Suffin broke.", "Whut?")
@@ -206,17 +224,18 @@
     def test_finish_suppresses_AMQPException(self):
         session = self.session_factory()
         session.defer(self.raise_AMQPException)
-        session.finish()
-        # Look, no exceptions!
+        session.finish()  # Look, no exceptions!
+        self.assertNoOops()
 
     def raise_MessagingException(self):
         raise MessagingException("Arm stuck in combine.")
 
     def test_finish_suppresses_MessagingException(self):
+        self.error_utility = ErrorReportingUtility()
         session = self.session_factory()
         session.defer(self.raise_MessagingException)
-        session.finish()
-        # Look, no exceptions!
+        session.finish()  # Look, no exceptions!
+        self.assertNoOops()
 
     def raise_IOError(self):
         raise IOError("Leg eaten by cow.")
@@ -224,16 +243,17 @@
     def test_finish_suppresses_IOError(self):
         session = self.session_factory()
         session.defer(self.raise_IOError)
-        session.finish()
-        # Look, no exceptions!
+        session.finish()  # Look, no exceptions!
+        self.assertNoOops()
 
     def raise_Exception(self):
         raise Exception("That hent worked.")
 
-    def test_finish_does_not_suppress_other_errors(self):
+    def test_finish_suppresses_other_errors_with_oopses(self):
         session = self.session_factory()
         session.defer(self.raise_Exception)
-        self.assertRaises(Exception, session.finish)
+        session.finish()  # Look, no exceptions!
+        self.assertOops("That hent worked.")
 
 
 class TestRabbitMessageBase(RabbitTestCase):