Launchpad itself

Merge lp:~lifeless/launchpad/oops into lp:launchpad

  1. oops
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: no longer in the source branch.
Merged at revision: 11538
Proposed branch: lp:~lifeless/launchpad/oops
Merge into: lp:launchpad
Diff against target: 313 lines (+70/-71)
6 files modified
lib/canonical/launchpad/doc/timeout.txt (+3/-3)
lib/canonical/launchpad/webapp/adapter.py (+55/-36)
lib/canonical/launchpad/webapp/configure.zcml (+1/-1)
lib/canonical/launchpad/webapp/ftests/test_adapter.txt (+2/-2)
lib/canonical/launchpad/webapp/servers.py (+0/-21)
lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt (+9/-8)
To merge this branch: bzr merge lp:~lifeless/launchpad/oops
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+35060@code.launchpad.net

Commit message

Consolidate timeout calculation code to reduce duplication.

Description of the change

Consolidate timeout calculation code to reduce duplication and give us one place to change/refactor things.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks good. Thanks for the drive-bys. Be sure to keep docstrings and function names non-confusing; a bit of laziness in documenting functions often pays off because you'll know exactly what the code needs to do in practice before you state its purpose.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/timeout.txt'
2--- lib/canonical/launchpad/doc/timeout.txt 2010-03-09 18:41:20 +0000
3+++ lib/canonical/launchpad/doc/timeout.txt 2010-09-13 04:47:46 +0000
4@@ -10,7 +10,7 @@
5 time remaining before the request should time out.
6
7 >>> from canonical.lazr.timeout import get_default_timeout_function
8- >>> from canonical.launchpad.webapp.servers import (
9+ >>> from canonical.launchpad.webapp.adapter import (
10 ... set_launchpad_default_timeout)
11 >>> old_func = get_default_timeout_function()
12
13@@ -21,7 +21,7 @@
14 >>> set_launchpad_default_timeout(ProcessStarting())
15
16 >>> get_default_timeout_function()
17- <function launchpad_default_timeout...>
18+ <function get_request_remaining_seconds...>
19
20 The timeout to use is the number of seconds remaining before
21 db_statement_timeout is expired.
22@@ -30,7 +30,7 @@
23 >>> from canonical.config import config
24 >>> config.push('timeout', dedent('''\
25 ... [database]
26- ... db_statement_timeout = 10'''))
27+ ... db_statement_timeout = 10000'''))
28
29 >>> timeout_func = get_default_timeout_function()
30
31
32=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
33--- lib/canonical/launchpad/webapp/adapter.py 2010-09-01 08:48:55 +0000
34+++ lib/canonical/launchpad/webapp/adapter.py 2010-09-13 04:47:46 +0000
35@@ -66,6 +66,7 @@
36 )
37 from canonical.launchpad.webapp.opstats import OpStats
38 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
39+from canonical.lazr.timeout import set_default_timeout_function
40 from lp.services.timeline.timeline import Timeline
41 from lp.services.timeline.requesttimeline import (
42 get_request_timeline,
43@@ -82,6 +83,7 @@
44 'get_request_duration',
45 'get_store_name',
46 'hard_timeout_expired',
47+ 'launchpad_default_timeout',
48 'soft_timeout_expired',
49 'StoreSelector',
50 ]
51@@ -104,11 +106,18 @@
52 return ('Statement: %r\nParameters:%r\nOriginal error: %r'
53 % (self.statement, self.params, self.original_error))
54
55+
56+class RequestExpired(RuntimeError):
57+ """Request has timed out."""
58+ implements(IRequestExpired)
59+
60+
61 def _get_dirty_commit_flags():
62 """Return the current dirty commit status"""
63 from canonical.ftests.pgsql import ConnectionWrapper
64 return (ConnectionWrapper.committed, ConnectionWrapper.dirty)
65
66+
67 def _reset_dirty_commit_flags(previous_committed, previous_dirty):
68 """Set the dirty commit status to False unless previous is True"""
69 from canonical.ftests.pgsql import ConnectionWrapper
70@@ -248,32 +257,48 @@
71 return now - starttime
72
73
74-def _check_expired(timeout):
75- """Checks whether the current request has passed the given timeout."""
76- if timeout is None or not getattr(_local, 'enable_timeout', True):
77- return False # no timeout configured or timeout disabled.
78-
79- starttime = getattr(_local, 'request_start_time', None)
80- if starttime is None:
81- return False # no current request
82-
83- requesttime = (time() - starttime) * 1000
84- return requesttime > timeout
85-
86-
87-def hard_timeout_expired():
88- """Returns True if the hard request timeout been reached."""
89- return _check_expired(config.database.db_statement_timeout)
90+def get_request_remaining_seconds(no_exception=False, now=None, timeout=None):
91+ """Return how many seconds are remaining in the current request budget.
92+
93+ If timouts are disabled, None is returned.
94+
95+ :param no_exception: If True, do not raise an error if the request
96+ is out of time. Instead return a float e.g. -2.0 for 2 seconds over
97+ budget.
98+ :param now: Override the result of time.time()
99+ :param timeout: A custom timeout in ms.
100+ :return: None or a float representing the remaining time budget.
101+ """
102+ if not getattr(_local, 'enable_timeout', True):
103+ return None
104+ if timeout is None:
105+ timeout = config.database.db_statement_timeout
106+ if not timeout:
107+ return None
108+ duration = get_request_duration(now)
109+ if duration == -1:
110+ return None
111+ remaining = timeout / 1000.0 - duration
112+ if remaining <= 0:
113+ if no_exception:
114+ return remaining
115+ raise RequestExpired('request expired.')
116+ return remaining
117+
118+
119+def set_launchpad_default_timeout(event):
120+ """Set the LAZR default timeout function on IProcessStartingEvent."""
121+ set_default_timeout_function(get_request_remaining_seconds)
122
123
124 def soft_timeout_expired():
125 """Returns True if the soft request timeout been reached."""
126- return _check_expired(config.database.soft_request_timeout)
127-
128-
129-class RequestExpired(RuntimeError):
130- """Request has timed out."""
131- implements(IRequestExpired)
132+ try:
133+ get_request_remaining_seconds(
134+ timeout=config.database.soft_request_timeout)
135+ return False
136+ except RequestExpired:
137+ return True
138
139
140 # ---- Prevent database access in the main thread of the app server
141@@ -468,7 +493,7 @@
142
143 @property
144 def granularity(self):
145- return dbconfig.db_statement_timeout_precision / 1000.0
146+ return config.database.db_statement_timeout_precision / 1000.0
147
148 def connection_raw_execute(self, connection, raw_cursor,
149 statement, params):
150@@ -478,15 +503,17 @@
151 if not isinstance(connection._database, LaunchpadDatabase):
152 return
153 # If we are outside of a request, don't do timeout adjustment.
154- if self.get_remaining_time() is None:
155- return
156 try:
157+ if self.get_remaining_time() is None:
158+ return
159 super(LaunchpadTimeoutTracer, self).connection_raw_execute(
160 connection, raw_cursor, statement, params)
161- except TimeoutError:
162+ except (RequestExpired, TimeoutError):
163+ # XXX: This code does not belong here - see bug=636804.
164+ # Robert Collins 20100913.
165+ OpStats.stats['timeouts'] += 1
166 info = sys.exc_info()
167 transaction.doom()
168- OpStats.stats['timeouts'] += 1
169 try:
170 raise info[0], info[1], info[2]
171 finally:
172@@ -505,15 +532,7 @@
173
174 def get_remaining_time(self):
175 """See `TimeoutTracer`"""
176- if (not dbconfig.db_statement_timeout or
177- not getattr(_local, 'enable_timeout', True)):
178- return None
179- start_time = getattr(_local, 'request_start_time', None)
180- if start_time is None:
181- return None
182- now = time()
183- ellapsed = now - start_time
184- return dbconfig.db_statement_timeout / 1000.0 - ellapsed
185+ return get_request_remaining_seconds()
186
187
188 class LaunchpadStatementTracer:
189
190=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
191--- lib/canonical/launchpad/webapp/configure.zcml 2010-08-13 14:42:58 +0000
192+++ lib/canonical/launchpad/webapp/configure.zcml 2010-09-13 04:47:46 +0000
193@@ -785,7 +785,7 @@
194 <!-- Set the default timeout function. -->
195 <subscriber
196 for="zope.app.appsetup.IProcessStartingEvent"
197- handler="canonical.launchpad.webapp.servers.set_launchpad_default_timeout"
198+ handler="canonical.launchpad.webapp.adapter.set_launchpad_default_timeout"
199 />
200
201 <subscriber
202
203=== modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.txt'
204--- lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-09-01 07:50:21 +0000
205+++ lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-09-13 04:47:46 +0000
206@@ -296,7 +296,7 @@
207 >>> store.execute('SELECT 2', noresult=True)
208 Traceback (most recent call last):
209 ...
210- TimeoutError: ...SELECT 2...
211+ RequestExpired: request expired.
212
213 The statement about to be executed is not recorded in the statement log.
214 The request time limit was exceeded before the statement was issued to
215@@ -359,7 +359,7 @@
216 >>> store.execute('SELECT 1', noresult=True)
217 Traceback (most recent call last):
218 ...
219- TimeoutError: ...SELECT 1...
220+ RequestExpired: request expired.
221 >>> statement_issued.set()
222 >>> thread.join()
223 >>> clear_request_started()
224
225=== modified file 'lib/canonical/launchpad/webapp/servers.py'
226--- lib/canonical/launchpad/webapp/servers.py 2010-09-04 09:57:29 +0000
227+++ lib/canonical/launchpad/webapp/servers.py 2010-09-13 04:47:46 +0000
228@@ -76,10 +76,6 @@
229 TimestampOrderingError,
230 )
231 import canonical.launchpad.layers
232-from canonical.launchpad.webapp.adapter import (
233- get_request_duration,
234- RequestExpired,
235- )
236 from canonical.launchpad.webapp.authentication import (
237 check_oauth_signature,
238 get_oauth_authorization,
239@@ -113,7 +109,6 @@
240 )
241 from canonical.launchpad.webapp.vhosts import allvhosts
242 from canonical.lazr.interfaces.feed import IFeed
243-from canonical.lazr.timeout import set_default_timeout_function
244 from lp.app.errors import UnexpectedFormData
245 from lp.services.features.flags import NullFeatureController
246 from lp.services.propertycache import cachedproperty
247@@ -1489,22 +1484,6 @@
248 return "Protocol error: %s" % self.status
249
250
251-def launchpad_default_timeout():
252- """Return the time before the request should be expired."""
253- timeout = config.database.db_statement_timeout
254- if timeout is None:
255- return None
256- left = timeout - get_request_duration()
257- if left < 0:
258- raise RequestExpired('request expired.')
259- return left
260-
261-
262-def set_launchpad_default_timeout(event):
263- """Set the LAZR default timeout function."""
264- set_default_timeout_function(launchpad_default_timeout)
265-
266-
267 def register_launchpad_request_publication_factories():
268 """Register our factories with the Zope3 publisher.
269
270
271=== modified file 'lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt'
272--- lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt 2008-06-24 07:57:39 +0000
273+++ lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt 2010-09-13 04:47:46 +0000
274@@ -9,9 +9,8 @@
275 ... implements(IRequestExpired)
276
277 After a timeout has happened, we can no longer do any more db queries,
278-since _check_expired() returns True. However, if the user is logged in,
279-a query will be issued to render the "Logged in as No Privileges
280-Person".
281+since get_request_remaining_seconds will raise. However, if the user is logged
282+in, a query will be issued to render the "Logged in as No Privileges Person".
283
284 >>> from textwrap import dedent
285 >>> from canonical.config import config
286@@ -23,12 +22,14 @@
287
288 >>> from time import time
289 >>> from canonical.launchpad.webapp.adapter import (
290- ... _check_expired, set_request_started)
291+ ... get_request_remaining_seconds, set_request_started)
292 >>> login('no-priv@canonical.com')
293 >>> too_early = time() - (config.database.db_statement_timeout / 1000 + 1)
294 >>> set_request_started(too_early)
295- >>> _check_expired(config.database.db_statement_timeout)
296- True
297+ >>> get_request_remaining_seconds()
298+ Traceback (most recent call last):
299+ ...
300+ RequestExpired: request expired.
301
302 Before the OOPS page is rendered, the timeout is reset, so that we can
303 issue the DB query and render the page, showing the OOPS id to the user.
304@@ -44,8 +45,8 @@
305 >>> timeout_exception = TimeoutException()
306 >>> exception_view = getMultiAdapter(
307 ... (timeout_exception, request), name="index.html")
308- >>> _check_expired(config.database.db_statement_timeout)
309- False
310+ >>> get_request_remaining_seconds() > 0
311+ True
312 >>> print exception_view()
313 <...OOPS_ID_MARKER...
314