Merge lp:~bac/launchpad/bug-974617 into lp:launchpad

[Brad Crittenden]

After restarting the pgbouncer in a test, retry a few times to give it a chance to come up.

Parallel testing exposed a problem where bouncer.start() was returning
but the bouncer service was not actually up.

This fix is gentler, giving the bouncer more time to tidy up and get started.

[Richard Harding (rharding)]

Bac, looks like a fun issue. Couple of notes:

- What about having the retry count setup as a constant. DB_RETRIES or something in case it needs to be tweaked/adjusted.
- I'm not sure if this exception gets raised to a user or log, but it'd probably be helpful if it has some context to it. Maybe raising an EnvironmentError with a message that it failed to come up after X attempts?

review: Approve (code*)

[j.c.sackett (jcsackett)]

This looks good; I agree with Rick's request that the count be setup as a constant rather than a magic number in the iteration, but that's pretty minor.

review: Approve

=== modified file 'lib/lp/services/webapp/tests/test_error.py'
--- lib/lp/services/webapp/tests/test_error.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webapp/tests/test_error.py	2012-04-12 19:31:20 +0000
@@ -3,13 +3,15 @@
 
 """Test error views."""
 
-import urllib2
 
+import httplib
 from storm.exceptions import (
     DisconnectionError,
     OperationalError,
     )
+import time
 import transaction
+import urllib2
 
 from lp.services.webapp.error import (
     DisconnectionErrorView,
@@ -26,6 +28,10 @@
 from lp.testing.matchers import Contains
 
 
+class TimeoutException(Exception):
+    pass
+
+
 class TestSystemErrorView(TestCase):
 
     layer = LaunchpadFunctionalLayer
@@ -112,8 +118,25 @@
         # We keep seeing the correct exception on subsequent requests.
         self.assertEqual(503, self.getHTTPError(url).code)
         # When the database is available again, requests succeed.
-        bouncer.start()
-        urllib2.urlopen(url)
+        #bouncer.start()
+        # bouncer.start() can sometimes return before the service is actually
+        # available for use.  To be defensive, let's retry a few times.  See
+        # bug 974617.
+        retries = 5
+        for i in xrange(retries):
+            try:
+                urllib2.urlopen(url)
+            except urllib2.HTTPError as e:
+                if e.code != httplib.SERVICE_UNAVAILABLE:
+                    raise
+            else:
+                break
+            time.sleep(0.5)
+        else:
+            raise TimeoutException(
+                "bouncer did not come up after {} attempts.".format(retries))
+
+
 
     def test_operationalerror_view(self):
         request = LaunchpadTestRequest()