launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/oops into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #627940 of the blocking calls made only database statements are recorded in OOPS
  https://bugs.launchpad.net/bugs/627940
  #630612 Complete b0rkage of oops timing info
  https://bugs.launchpad.net/bugs/630612
  #632022 oops reports show negative total time
  https://bugs.launchpad.net/bugs/632022


Consolidate timeout calculation code to reduce duplication and give us one place to change/refactor things.
-- 
https://code.launchpad.net/~lifeless/launchpad/oops/+merge/35060
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/oops into lp:launchpad/devel.

=== 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-10 06:46:06 +0000
@@ -10,7 +10,7 @@
 time remaining before the request should time out.
 
     >>> from canonical.lazr.timeout import get_default_timeout_function
-    >>> from canonical.launchpad.webapp.servers import (
+    >>> from canonical.launchpad.webapp.adapter import (
     ...     set_launchpad_default_timeout)
     >>> old_func = get_default_timeout_function()
 

=== 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-10 06:46:06 +0000
@@ -66,6 +66,7 @@
     )
 from canonical.launchpad.webapp.opstats import OpStats
 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
+from canonical.lazr.timeout import set_default_timeout_function
 from lp.services.timeline.timeline import Timeline
 from lp.services.timeline.requesttimeline import (
     get_request_timeline,
@@ -82,6 +83,7 @@
     'get_request_duration',
     'get_store_name',
     'hard_timeout_expired',
+    'launchpad_default_timeout',
     'soft_timeout_expired',
     'StoreSelector',
     ]
@@ -104,11 +106,18 @@
         return ('Statement: %r\nParameters:%r\nOriginal error: %r'
                 % (self.statement, self.params, self.original_error))
 
+
+class RequestExpired(RuntimeError):
+    """Request has timed out."""
+    implements(IRequestExpired)
+
+
 def _get_dirty_commit_flags():
     """Return the current dirty commit status"""
     from canonical.ftests.pgsql import ConnectionWrapper
     return (ConnectionWrapper.committed, ConnectionWrapper.dirty)
 
+
 def _reset_dirty_commit_flags(previous_committed, previous_dirty):
     """Set the dirty commit status to False unless previous is True"""
     from canonical.ftests.pgsql import ConnectionWrapper
@@ -248,32 +257,54 @@
     return now - starttime
 
 
-def _check_expired(timeout):
+def get_request_remaining_seconds(no_exception=False, now=None, timeout=None):
+    """Return how many seconds are remaining in the current request budget.
+
+    If no timouts are enabled, this returns None.
+
+    If timeouts are enabled, this returns 0 if the budget is exhausted, or
+    remaining time in seconds.
+
+    :param no_exception: If True, do not raise an error if the request
+        is out of time. Instead return a float e.g. -2.0 for 2 seconds over
+        budget.
+    :param now: Override the result of time.time()
+    :param timeout: A custom timeout in ms.
+    :return: None or a float representing the remaining time budget.
+    """
+    if not getattr(_local, 'enable_timeout', True):
+        return None
+    if timeout is None:
+        timeout = config.database.db_statement_timeout
+    if not timeout:
+        return None
+    duration = get_request_duration(now)
+    if duration == -1:
+        return None
+    remaining = timeout / 1000.0 - duration
+    if remaining <= 0:
+        if no_exception:
+            return remaining
+        raise RequestExpired('request expired.')
+    return remaining
+
+
+def set_launchpad_default_timeout(event):
+    """Set the LAZR default timeout function."""
+    set_default_timeout_function(get_request_remaining_seconds)
+
+
+def _check_expired(timeout=None):
     """Checks whether the current request has passed the given timeout."""
-    if timeout is None or not getattr(_local, 'enable_timeout', True):
-        return False # no timeout configured or timeout disabled.
-
-    starttime = getattr(_local, 'request_start_time', None)
-    if starttime is None:
-        return False # no current request
-
-    requesttime = (time() - starttime) * 1000
-    return requesttime > timeout
-
-
-def hard_timeout_expired():
-    """Returns True if the hard request timeout been reached."""
-    return _check_expired(config.database.db_statement_timeout)
 
 
 def soft_timeout_expired():
     """Returns True if the soft request timeout been reached."""
-    return _check_expired(config.database.soft_request_timeout)
-
-
-class RequestExpired(RuntimeError):
-    """Request has timed out."""
-    implements(IRequestExpired)
+    try:
+        get_request_remaining_seconds(timeout=timeout)
+        return False
+    except RequestExpired:
+        return True
 
 
 # ---- Prevent database access in the main thread of the app server
@@ -468,7 +499,7 @@
 
     @property
     def granularity(self):
-        return dbconfig.db_statement_timeout_precision / 1000.0
+        return config.database.db_statement_timeout_precision / 1000.0
 
     def connection_raw_execute(self, connection, raw_cursor,
                                statement, params):
@@ -505,15 +536,7 @@
 
     def get_remaining_time(self):
         """See `TimeoutTracer`"""
-        if (not dbconfig.db_statement_timeout or
-            not getattr(_local, 'enable_timeout', True)):
-            return None
-        start_time = getattr(_local, 'request_start_time', None)
-        if start_time is None:
-            return None
-        now = time()
-        ellapsed = now - start_time
-        return  dbconfig.db_statement_timeout / 1000.0 - ellapsed
+        return get_request_remaining_seconds()
 
 
 class LaunchpadStatementTracer:

=== 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-10 06:46:06 +0000
@@ -785,7 +785,7 @@
     <!-- Set the default timeout function. -->
     <subscriber
         for="zope.app.appsetup.IProcessStartingEvent"
-        handler="canonical.launchpad.webapp.servers.set_launchpad_default_timeout"
+        handler="canonical.launchpad.webapp.adapter.set_launchpad_default_timeout"
         />
 
     <subscriber

=== 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-10 06:46:06 +0000
@@ -76,10 +76,6 @@
     TimestampOrderingError,
     )
 import canonical.launchpad.layers
-from canonical.launchpad.webapp.adapter import (
-    get_request_duration,
-    RequestExpired,
-    )
 from canonical.launchpad.webapp.authentication import (
     check_oauth_signature,
     get_oauth_authorization,
@@ -113,7 +109,6 @@
     )
 from canonical.launchpad.webapp.vhosts import allvhosts
 from canonical.lazr.interfaces.feed import IFeed
-from canonical.lazr.timeout import set_default_timeout_function
 from lp.app.errors import UnexpectedFormData
 from lp.services.features.flags import NullFeatureController
 from lp.services.propertycache import cachedproperty
@@ -1489,22 +1484,6 @@
         return "Protocol error: %s" % self.status
 
 
-def launchpad_default_timeout():
-    """Return the time before the request should be expired."""
-    timeout = config.database.db_statement_timeout
-    if timeout is None:
-        return None
-    left = timeout - get_request_duration()
-    if left < 0:
-        raise RequestExpired('request expired.')
-    return left
-
-
-def set_launchpad_default_timeout(event):
-    """Set the LAZR default timeout function."""
-    set_default_timeout_function(launchpad_default_timeout)
-
-
 def register_launchpad_request_publication_factories():
     """Register our factories with the Zope3 publisher.