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

Proposed by Brad Crittenden on 2012-04-12
Status: Merged
Approved by: Brad Crittenden on 2012-04-12
Approved revision: no longer in the source branch.
Merged at revision: 15086
Proposed branch: lp:~bac/launchpad/bug-974617
Merge into: lp:launchpad
Diff against target: 59 lines (+26/-3)
1 file modified
lib/lp/services/webapp/tests/test_error.py (+26/-3)
To merge this branch: bzr merge lp:~bac/launchpad/bug-974617
Reviewer Review Type Date Requested Status
j.c.sackett (community) 2012-04-12 Approve on 2012-04-12
Richard Harding (community) code* 2012-04-12 Approve on 2012-04-12
Review via email: mp+101797@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Richard Harding (rharding) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/services/webapp/tests/test_error.py'
2--- lib/lp/services/webapp/tests/test_error.py 2012-01-01 02:58:52 +0000
3+++ lib/lp/services/webapp/tests/test_error.py 2012-04-12 19:31:20 +0000
4@@ -3,13 +3,15 @@
5
6 """Test error views."""
7
8-import urllib2
9
10+import httplib
11 from storm.exceptions import (
12 DisconnectionError,
13 OperationalError,
14 )
15+import time
16 import transaction
17+import urllib2
18
19 from lp.services.webapp.error import (
20 DisconnectionErrorView,
21@@ -26,6 +28,10 @@
22 from lp.testing.matchers import Contains
23
24
25+class TimeoutException(Exception):
26+ pass
27+
28+
29 class TestSystemErrorView(TestCase):
30
31 layer = LaunchpadFunctionalLayer
32@@ -112,8 +118,25 @@
33 # We keep seeing the correct exception on subsequent requests.
34 self.assertEqual(503, self.getHTTPError(url).code)
35 # When the database is available again, requests succeed.
36- bouncer.start()
37- urllib2.urlopen(url)
38+ #bouncer.start()
39+ # bouncer.start() can sometimes return before the service is actually
40+ # available for use. To be defensive, let's retry a few times. See
41+ # bug 974617.
42+ retries = 5
43+ for i in xrange(retries):
44+ try:
45+ urllib2.urlopen(url)
46+ except urllib2.HTTPError as e:
47+ if e.code != httplib.SERVICE_UNAVAILABLE:
48+ raise
49+ else:
50+ break
51+ time.sleep(0.5)
52+ else:
53+ raise TimeoutException(
54+ "bouncer did not come up after {} attempts.".format(retries))
55+
56+
57
58 def test_operationalerror_view(self):
59 request = LaunchpadTestRequest()