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
=== modified file 'lib/canonical/launchpad/doc/timeout.txt'
--- lib/canonical/launchpad/doc/timeout.txt 2010-03-09 18:41:20 +0000
+++ lib/canonical/launchpad/doc/timeout.txt 2010-09-13 04:47:46 +0000
@@ -10,7 +10,7 @@
10time remaining before the request should time out.10time remaining before the request should time out.
1111
12 >>> from canonical.lazr.timeout import get_default_timeout_function12 >>> from canonical.lazr.timeout import get_default_timeout_function
13 >>> from canonical.launchpad.webapp.servers import (13 >>> from canonical.launchpad.webapp.adapter import (
14 ... set_launchpad_default_timeout)14 ... set_launchpad_default_timeout)
15 >>> old_func = get_default_timeout_function()15 >>> old_func = get_default_timeout_function()
1616
@@ -21,7 +21,7 @@
21 >>> set_launchpad_default_timeout(ProcessStarting())21 >>> set_launchpad_default_timeout(ProcessStarting())
2222
23 >>> get_default_timeout_function()23 >>> get_default_timeout_function()
24 <function launchpad_default_timeout...>24 <function get_request_remaining_seconds...>
2525
26The timeout to use is the number of seconds remaining before26The timeout to use is the number of seconds remaining before
27db_statement_timeout is expired.27db_statement_timeout is expired.
@@ -30,7 +30,7 @@
30 >>> from canonical.config import config30 >>> from canonical.config import config
31 >>> config.push('timeout', dedent('''\31 >>> config.push('timeout', dedent('''\
32 ... [database]32 ... [database]
33 ... db_statement_timeout = 10'''))33 ... db_statement_timeout = 10000'''))
3434
35 >>> timeout_func = get_default_timeout_function()35 >>> timeout_func = get_default_timeout_function()
3636
3737
=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py 2010-09-01 08:48:55 +0000
+++ lib/canonical/launchpad/webapp/adapter.py 2010-09-13 04:47:46 +0000
@@ -66,6 +66,7 @@
66 )66 )
67from canonical.launchpad.webapp.opstats import OpStats67from canonical.launchpad.webapp.opstats import OpStats
68from canonical.lazr.utils import get_current_browser_request, safe_hasattr68from canonical.lazr.utils import get_current_browser_request, safe_hasattr
69from canonical.lazr.timeout import set_default_timeout_function
69from lp.services.timeline.timeline import Timeline70from lp.services.timeline.timeline import Timeline
70from lp.services.timeline.requesttimeline import (71from lp.services.timeline.requesttimeline import (
71 get_request_timeline,72 get_request_timeline,
@@ -82,6 +83,7 @@
82 'get_request_duration',83 'get_request_duration',
83 'get_store_name',84 'get_store_name',
84 'hard_timeout_expired',85 'hard_timeout_expired',
86 'launchpad_default_timeout',
85 'soft_timeout_expired',87 'soft_timeout_expired',
86 'StoreSelector',88 'StoreSelector',
87 ]89 ]
@@ -104,11 +106,18 @@
104 return ('Statement: %r\nParameters:%r\nOriginal error: %r'106 return ('Statement: %r\nParameters:%r\nOriginal error: %r'
105 % (self.statement, self.params, self.original_error))107 % (self.statement, self.params, self.original_error))
106108
109
110class RequestExpired(RuntimeError):
111 """Request has timed out."""
112 implements(IRequestExpired)
113
114
107def _get_dirty_commit_flags():115def _get_dirty_commit_flags():
108 """Return the current dirty commit status"""116 """Return the current dirty commit status"""
109 from canonical.ftests.pgsql import ConnectionWrapper117 from canonical.ftests.pgsql import ConnectionWrapper
110 return (ConnectionWrapper.committed, ConnectionWrapper.dirty)118 return (ConnectionWrapper.committed, ConnectionWrapper.dirty)
111119
120
112def _reset_dirty_commit_flags(previous_committed, previous_dirty):121def _reset_dirty_commit_flags(previous_committed, previous_dirty):
113 """Set the dirty commit status to False unless previous is True"""122 """Set the dirty commit status to False unless previous is True"""
114 from canonical.ftests.pgsql import ConnectionWrapper123 from canonical.ftests.pgsql import ConnectionWrapper
@@ -248,32 +257,48 @@
248 return now - starttime257 return now - starttime
249258
250259
251def _check_expired(timeout):260def get_request_remaining_seconds(no_exception=False, now=None, timeout=None):
252 """Checks whether the current request has passed the given timeout."""261 """Return how many seconds are remaining in the current request budget.
253 if timeout is None or not getattr(_local, 'enable_timeout', True):262
254 return False # no timeout configured or timeout disabled.263 If timouts are disabled, None is returned.
255264
256 starttime = getattr(_local, 'request_start_time', None)265 :param no_exception: If True, do not raise an error if the request
257 if starttime is None:266 is out of time. Instead return a float e.g. -2.0 for 2 seconds over
258 return False # no current request267 budget.
259268 :param now: Override the result of time.time()
260 requesttime = (time() - starttime) * 1000269 :param timeout: A custom timeout in ms.
261 return requesttime > timeout270 :return: None or a float representing the remaining time budget.
262271 """
263272 if not getattr(_local, 'enable_timeout', True):
264def hard_timeout_expired():273 return None
265 """Returns True if the hard request timeout been reached."""274 if timeout is None:
266 return _check_expired(config.database.db_statement_timeout)275 timeout = config.database.db_statement_timeout
276 if not timeout:
277 return None
278 duration = get_request_duration(now)
279 if duration == -1:
280 return None
281 remaining = timeout / 1000.0 - duration
282 if remaining <= 0:
283 if no_exception:
284 return remaining
285 raise RequestExpired('request expired.')
286 return remaining
287
288
289def set_launchpad_default_timeout(event):
290 """Set the LAZR default timeout function on IProcessStartingEvent."""
291 set_default_timeout_function(get_request_remaining_seconds)
267292
268293
269def soft_timeout_expired():294def soft_timeout_expired():
270 """Returns True if the soft request timeout been reached."""295 """Returns True if the soft request timeout been reached."""
271 return _check_expired(config.database.soft_request_timeout)296 try:
272297 get_request_remaining_seconds(
273298 timeout=config.database.soft_request_timeout)
274class RequestExpired(RuntimeError):299 return False
275 """Request has timed out."""300 except RequestExpired:
276 implements(IRequestExpired)301 return True
277302
278303
279# ---- Prevent database access in the main thread of the app server304# ---- Prevent database access in the main thread of the app server
@@ -468,7 +493,7 @@
468493
469 @property494 @property
470 def granularity(self):495 def granularity(self):
471 return dbconfig.db_statement_timeout_precision / 1000.0496 return config.database.db_statement_timeout_precision / 1000.0
472497
473 def connection_raw_execute(self, connection, raw_cursor,498 def connection_raw_execute(self, connection, raw_cursor,
474 statement, params):499 statement, params):
@@ -478,15 +503,17 @@
478 if not isinstance(connection._database, LaunchpadDatabase):503 if not isinstance(connection._database, LaunchpadDatabase):
479 return504 return
480 # If we are outside of a request, don't do timeout adjustment.505 # If we are outside of a request, don't do timeout adjustment.
481 if self.get_remaining_time() is None:
482 return
483 try:506 try:
507 if self.get_remaining_time() is None:
508 return
484 super(LaunchpadTimeoutTracer, self).connection_raw_execute(509 super(LaunchpadTimeoutTracer, self).connection_raw_execute(
485 connection, raw_cursor, statement, params)510 connection, raw_cursor, statement, params)
486 except TimeoutError:511 except (RequestExpired, TimeoutError):
512 # XXX: This code does not belong here - see bug=636804.
513 # Robert Collins 20100913.
514 OpStats.stats['timeouts'] += 1
487 info = sys.exc_info()515 info = sys.exc_info()
488 transaction.doom()516 transaction.doom()
489 OpStats.stats['timeouts'] += 1
490 try:517 try:
491 raise info[0], info[1], info[2]518 raise info[0], info[1], info[2]
492 finally:519 finally:
@@ -505,15 +532,7 @@
505532
506 def get_remaining_time(self):533 def get_remaining_time(self):
507 """See `TimeoutTracer`"""534 """See `TimeoutTracer`"""
508 if (not dbconfig.db_statement_timeout or535 return get_request_remaining_seconds()
509 not getattr(_local, 'enable_timeout', True)):
510 return None
511 start_time = getattr(_local, 'request_start_time', None)
512 if start_time is None:
513 return None
514 now = time()
515 ellapsed = now - start_time
516 return dbconfig.db_statement_timeout / 1000.0 - ellapsed
517536
518537
519class LaunchpadStatementTracer:538class LaunchpadStatementTracer:
520539
=== modified file 'lib/canonical/launchpad/webapp/configure.zcml'
--- lib/canonical/launchpad/webapp/configure.zcml 2010-08-13 14:42:58 +0000
+++ lib/canonical/launchpad/webapp/configure.zcml 2010-09-13 04:47:46 +0000
@@ -785,7 +785,7 @@
785 <!-- Set the default timeout function. -->785 <!-- Set the default timeout function. -->
786 <subscriber786 <subscriber
787 for="zope.app.appsetup.IProcessStartingEvent"787 for="zope.app.appsetup.IProcessStartingEvent"
788 handler="canonical.launchpad.webapp.servers.set_launchpad_default_timeout"788 handler="canonical.launchpad.webapp.adapter.set_launchpad_default_timeout"
789 />789 />
790790
791 <subscriber791 <subscriber
792792
=== modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.txt'
--- lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-09-01 07:50:21 +0000
+++ lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-09-13 04:47:46 +0000
@@ -296,7 +296,7 @@
296 >>> store.execute('SELECT 2', noresult=True)296 >>> store.execute('SELECT 2', noresult=True)
297 Traceback (most recent call last):297 Traceback (most recent call last):
298 ...298 ...
299 TimeoutError: ...SELECT 2...299 RequestExpired: request expired.
300300
301The statement about to be executed is not recorded in the statement log.301The statement about to be executed is not recorded in the statement log.
302The request time limit was exceeded before the statement was issued to302The request time limit was exceeded before the statement was issued to
@@ -359,7 +359,7 @@
359 >>> store.execute('SELECT 1', noresult=True)359 >>> store.execute('SELECT 1', noresult=True)
360 Traceback (most recent call last):360 Traceback (most recent call last):
361 ...361 ...
362 TimeoutError: ...SELECT 1...362 RequestExpired: request expired.
363 >>> statement_issued.set()363 >>> statement_issued.set()
364 >>> thread.join()364 >>> thread.join()
365 >>> clear_request_started()365 >>> clear_request_started()
366366
=== modified file 'lib/canonical/launchpad/webapp/servers.py'
--- lib/canonical/launchpad/webapp/servers.py 2010-09-04 09:57:29 +0000
+++ lib/canonical/launchpad/webapp/servers.py 2010-09-13 04:47:46 +0000
@@ -76,10 +76,6 @@
76 TimestampOrderingError,76 TimestampOrderingError,
77 )77 )
78import canonical.launchpad.layers78import canonical.launchpad.layers
79from canonical.launchpad.webapp.adapter import (
80 get_request_duration,
81 RequestExpired,
82 )
83from canonical.launchpad.webapp.authentication import (79from canonical.launchpad.webapp.authentication import (
84 check_oauth_signature,80 check_oauth_signature,
85 get_oauth_authorization,81 get_oauth_authorization,
@@ -113,7 +109,6 @@
113 )109 )
114from canonical.launchpad.webapp.vhosts import allvhosts110from canonical.launchpad.webapp.vhosts import allvhosts
115from canonical.lazr.interfaces.feed import IFeed111from canonical.lazr.interfaces.feed import IFeed
116from canonical.lazr.timeout import set_default_timeout_function
117from lp.app.errors import UnexpectedFormData112from lp.app.errors import UnexpectedFormData
118from lp.services.features.flags import NullFeatureController113from lp.services.features.flags import NullFeatureController
119from lp.services.propertycache import cachedproperty114from lp.services.propertycache import cachedproperty
@@ -1489,22 +1484,6 @@
1489 return "Protocol error: %s" % self.status1484 return "Protocol error: %s" % self.status
14901485
14911486
1492def launchpad_default_timeout():
1493 """Return the time before the request should be expired."""
1494 timeout = config.database.db_statement_timeout
1495 if timeout is None:
1496 return None
1497 left = timeout - get_request_duration()
1498 if left < 0:
1499 raise RequestExpired('request expired.')
1500 return left
1501
1502
1503def set_launchpad_default_timeout(event):
1504 """Set the LAZR default timeout function."""
1505 set_default_timeout_function(launchpad_default_timeout)
1506
1507
1508def register_launchpad_request_publication_factories():1487def register_launchpad_request_publication_factories():
1509 """Register our factories with the Zope3 publisher.1488 """Register our factories with the Zope3 publisher.
15101489
15111490
=== modified file 'lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt'
--- lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt 2008-06-24 07:57:39 +0000
+++ lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt 2010-09-13 04:47:46 +0000
@@ -9,9 +9,8 @@
9 ... implements(IRequestExpired)9 ... implements(IRequestExpired)
1010
11After a timeout has happened, we can no longer do any more db queries,11After a timeout has happened, we can no longer do any more db queries,
12since _check_expired() returns True. However, if the user is logged in,12since get_request_remaining_seconds will raise. However, if the user is logged
13a query will be issued to render the "Logged in as No Privileges13in, a query will be issued to render the "Logged in as No Privileges Person".
14Person".
1514
16 >>> from textwrap import dedent15 >>> from textwrap import dedent
17 >>> from canonical.config import config16 >>> from canonical.config import config
@@ -23,12 +22,14 @@
2322
24 >>> from time import time23 >>> from time import time
25 >>> from canonical.launchpad.webapp.adapter import (24 >>> from canonical.launchpad.webapp.adapter import (
26 ... _check_expired, set_request_started)25 ... get_request_remaining_seconds, set_request_started)
27 >>> login('no-priv@canonical.com')26 >>> login('no-priv@canonical.com')
28 >>> too_early = time() - (config.database.db_statement_timeout / 1000 + 1)27 >>> too_early = time() - (config.database.db_statement_timeout / 1000 + 1)
29 >>> set_request_started(too_early)28 >>> set_request_started(too_early)
30 >>> _check_expired(config.database.db_statement_timeout)29 >>> get_request_remaining_seconds()
31 True30 Traceback (most recent call last):
31 ...
32 RequestExpired: request expired.
3233
33Before the OOPS page is rendered, the timeout is reset, so that we can34Before the OOPS page is rendered, the timeout is reset, so that we can
34issue the DB query and render the page, showing the OOPS id to the user.35issue the DB query and render the page, showing the OOPS id to the user.
@@ -44,8 +45,8 @@
44 >>> timeout_exception = TimeoutException()45 >>> timeout_exception = TimeoutException()
45 >>> exception_view = getMultiAdapter(46 >>> exception_view = getMultiAdapter(
46 ... (timeout_exception, request), name="index.html")47 ... (timeout_exception, request), name="index.html")
47 >>> _check_expired(config.database.db_statement_timeout)48 >>> get_request_remaining_seconds() > 0
48 False49 True
49 >>> print exception_view()50 >>> print exception_view()
50 <...OOPS_ID_MARKER...51 <...OOPS_ID_MARKER...
5152