launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/unbreak-api-timeouts into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #740750 in Launchpad itself: "API timeouts broken and returning no useful data..."
  https://bugs.launchpad.net/launchpad/+bug/740750

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/unbreak-api-timeouts/+merge/58226

Once upon a time, timeout page renders themselves timed out when querying the logged in user. So RequestExpiredView.__init__ was taught to reset the remaining time.

Somewhat later, in r7184, this was fixed for all exception types by resetting the timeout in the publication's beginErrorHandlingTransaction. The hack in RequestExpiredView.__init__ was now superfluous.

A month or so ago lazr.restful's exception handling changed. It now gets and instantiates the relevant view to check if it's a webservice exception view. If it is, it is handled internally. If not, it is raised up to the main webapp publisher. But instantiating RequestExpiredView there erases the request timeline before it can be saved, so API timeouts have no statement log -- bug #740750.

This branch is just a quick fix, removing the superfluous request timeline reset. The relevant test was also removed, since the behaviour has changed and the global publication reset is tested in test_adapter_timeout.txt.

API and webapp timeouts and other exception types still work fine, even with statement logs this time.

I will discuss with Zope people whether lazr.restful should be instantiating these views, but it doesn't really matter right now as this code is redundant anyway.
-- 
https://code.launchpad.net/~wgrant/launchpad/unbreak-api-timeouts/+merge/58226
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/unbreak-api-timeouts into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/error.py'
--- lib/canonical/launchpad/webapp/error.py	2011-04-06 05:40:04 +0000
+++ lib/canonical/launchpad/webapp/error.py	2011-04-19 04:27:29 +0000
@@ -241,10 +241,6 @@
         # is really just a guess and I don't think any clients actually
         # pay attention to it - it is just a hint.
         request.response.setHeader('Retry-After', 900)
-        # Reset the timeout timer, so that we can issue db queries when
-        # rendering the page.
-        clear_request_started()
-        set_request_started()
 
 
 class InvalidBatchSizeView(SystemErrorView):

=== removed file 'lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt'
--- lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/webapp/tests/test_request_expire_render.txt	1970-01-01 00:00:00 +0000
@@ -1,55 +0,0 @@
-= Rendering timeout exceptions =
-
-When a page takes too long to render, a timeout exception is raised. A
-timeout exception should provide IRequestExpired.
-
-    >>> from zope.interface import implements
-    >>> from canonical.database.interfaces import IRequestExpired
-    >>> class TimeoutException:
-    ...     implements(IRequestExpired)
-
-After a timeout has happened, we can no longer do any more db queries,
-since get_request_remaining_seconds will raise. However, if the user is logged
-in, a query will be issued to render the "Logged in as No Privileges Person".
-
-    >>> from canonical.config import config
-    >>> from textwrap import dedent
-    >>> test_data = dedent("""
-    ...     [database]
-    ...     db_statement_timeout: 10000
-    ...     """)
-    >>> config.push('test_data', test_data)
-
-    >>> from time import time
-    >>> from canonical.launchpad.webapp.adapter import (
-    ...     get_request_remaining_seconds, set_request_started)
-    >>> login('no-priv@xxxxxxxxxxxxx')
-    >>> too_early = time() - (config.database.db_statement_timeout / 1000 + 1)
-    >>> set_request_started(too_early)
-    >>> get_request_remaining_seconds()
-    Traceback (most recent call last):
-    ...
-    RequestExpired: request expired.
-
-Before the OOPS page is rendered, the timeout is reset, so that we can
-issue the DB query and render the page, showing the OOPS id to the user.
-
-    >>> from canonical.launchpad.webapp.interaction import (
-    ...     get_current_principal)
-    >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-    >>> request = LaunchpadTestRequest(environ={'PATH_INFO': '/'})
-    >>> request.oopsid = 'OOPS_ID_MARKER'
-    >>> request.setPrincipal(get_current_principal())
-
-    >>> from zope.component import getMultiAdapter
-    >>> timeout_exception = TimeoutException()
-    >>> exception_view = getMultiAdapter(
-    ...     (timeout_exception, request), name="index.html")
-    >>> get_request_remaining_seconds() > 0
-    True
-    >>> print exception_view()
-    <...OOPS_ID_MARKER...
-
-    >>> test_config_data = config.pop('test_data')
-    >>> from canonical.launchpad.webapp.adapter import clear_request_started
-    >>> clear_request_started()