launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04665
[Merge] lp:~lifeless/launchpad/useoops into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/useoops into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71978
Migrate loggerhead's oops integration to oops-wsgi.
--
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71978
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/useoops into lp:launchpad.
=== modified file 'lib/launchpad_loggerhead/app.py'
--- lib/launchpad_loggerhead/app.py 2011-08-09 14:53:07 +0000
+++ lib/launchpad_loggerhead/app.py 2011-08-18 03:43:26 +0000
@@ -11,18 +11,19 @@
from bzrlib import errors, lru_cache, urlutils
from bzrlib.transport import get_transport
-
from loggerhead.apps import favicon_app, static_app
from loggerhead.apps.branch import BranchWSGIApp
-
+import oops_wsgi
+from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
from openid.extensions.sreg import SRegRequest, SRegResponse
-from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
-
from paste import httpserver
from paste.fileapp import DataApp
+from paste.httpexceptions import (
+ HTTPMovedPermanently,
+ HTTPNotFound,
+ HTTPUnauthorized,
+ )
from paste.request import construct_url, parse_querystring, path_info_pop
-from paste.httpexceptions import (
- HTTPMovedPermanently, HTTPNotFound, HTTPUnauthorized)
from canonical.config import config
from canonical.launchpad.xmlrpc import faults
@@ -234,13 +235,6 @@
lp_server.stop_server()
-def make_oops_logging_exception_hook(error_utility, request):
- """Make a hook for logging OOPSes."""
- def log_oops():
- error_utility.raising(sys.exc_info(), request)
- return log_oops
-
-
def make_error_utility():
"""Make an error utility for logging errors from codebrowse."""
error_utility = ErrorReportingUtility()
@@ -265,72 +259,6 @@
</p></body></html>'''
-_error_status = '500 Internal Server Error'
-_error_headers = [('Content-Type', 'text/html')]
-
-
-class WrappedStartResponse(object):
- """Wraps start_response (from a WSGI request) to keep track of whether
- start_response was called (and whether the callable it returns has been
- called).
-
- Used by oops_middleware.
- """
-
- def __init__(self, start_response):
- self._real_start_response = start_response
- self.response_start = None
- self._write_callable = None
-
- @property
- def body_started(self):
- return self._write_callable is not None
-
- def start_response(self, status, headers, exc_info=None):
- # Just keep a note of the values we were called with for now. We don't
- # need to invoke the real start_response until the response body
- # starts.
- self.response_start = (status, headers)
- if exc_info is not None:
- self.response_start += (exc_info,)
- return self.write_wrapper
-
- def ensure_started(self):
- if not self.body_started and self.response_start is not None:
- self._really_start()
-
- def _really_start(self):
- self._write_callable = self._real_start_response(*self.response_start)
-
- def write_wrapper(self, data):
- self.ensure_started()
- self._write_callable(data)
-
- def generate_oops(self, environ, error_utility):
- """Generate an OOPS.
-
- :returns: True if the error page was sent to the user, and False if it
- couldn't (i.e. if the response body was already started).
- """
- oopsid = report_oops(environ, error_utility)
- if self.body_started:
- return False
- write = self.start_response(_error_status, _error_headers)
- write(_oops_html_template % {'oopsid': oopsid})
- return True
-
-
-def report_oops(environ, error_utility):
- # XXX: We should capture more per-request information to include in
- # the OOPS here, e.g. duration, user, etc. But even an OOPS with
- # just a traceback and URL is better than nothing.
- # - Andrew Bennetts, 2010-07-27.
- request = ScriptRequest(
- [], URL=construct_url(environ))
- error_utility.raising(sys.exc_info(), request)
- return request.oopsid
-
-
def oops_middleware(app):
"""Middleware to log an OOPS if the request fails.
@@ -338,43 +266,5 @@
a basic HTML error page with the OOPS ID to the user (and status code 500).
"""
error_utility = make_error_utility()
- def wrapped_app(environ, start_response):
- wrapped = WrappedStartResponse(start_response)
- try:
- # Start processing this request, build the app
- app_iter = iter(app(environ, wrapped.start_response))
- # Start yielding the response
- stopping = False
- while not stopping:
- try:
- data = app_iter.next()
- except StopIteration:
- stopping = True
- wrapped.ensure_started()
- if not stopping:
- yield data
- except httpserver.SocketErrors, e:
- # The Paste WSGIHandler suppresses these exceptions.
- # Generally it means something like 'EPIPE' because the
- # connection was closed. We don't want to generate an OOPS
- # just because the connection was closed prematurely.
- logger = logging.getLogger('lp-loggerhead')
- logger.info('Caught socket exception from %s: %s %s'
- % (environ.get('REMOTE_ADDR', '<unknown>'),
- e.__class__, e,))
- return
- except GeneratorExit, e:
- # This generally means a client closed early during a streaming
- # body. Nothing to worry about. GeneratorExit doesn't usually have
- # any context associated with it, so not worth printing to the log.
- logger = logging.getLogger('lp-loggerhead')
- logger.info('Caught GeneratorExit from %s'
- % (environ.get('REMOTE_ADDR', '<unknown>')))
- return
- except:
- error_page_sent = wrapped.generate_oops(environ, error_utility)
- if error_page_sent:
- return
- # Could not send error page to user, so... just give up.
- raise
- return wrapped_app
+ return oops_wsgi.make_app(app, error_utility._oops_config,
+ template=_oops_html_template)
=== modified file 'lib/launchpad_loggerhead/tests.py'
--- lib/launchpad_loggerhead/tests.py 2011-08-13 04:07:10 +0000
+++ lib/launchpad_loggerhead/tests.py 2011-08-18 03:43:26 +0000
@@ -20,8 +20,6 @@
from canonical.launchpad.webapp.vhosts import allvhosts
from canonical.testing.layers import DatabaseFunctionalLayer
from launchpad_loggerhead.app import (
- _oops_html_template,
- oops_middleware,
RootApp,
)
from launchpad_loggerhead.session import SessionHandler
@@ -150,154 +148,3 @@
self.assertEqual(self.browser.url, dummy_root + '+logout')
self.assertEqual(self.browser.contents,
'This is a dummy destination.\n')
-
-
-class TestOopsMiddleware(TestCase):
-
- def setUp(self):
- super(TestOopsMiddleware, self).setUp()
- self.start_response_called = False
-
- def assertContainsRe(self, haystack, needle_re, flags=0):
- """Assert that a contains something matching a regular expression."""
- # There is: self.assertTextMatchesExpressionIgnoreWhitespace
- # but it does weird things with whitespace, and gives
- # unhelpful error messages when it fails, so this is copied
- # from bzrlib
- if not re.search(needle_re, haystack, flags):
- if '\n' in haystack or len(haystack) > 60:
- # a long string, format it in a more readable way
- raise AssertionError(
- 'pattern "%s" not found in\n"""\\\n%s"""\n'
- % (needle_re, haystack))
- else:
- raise AssertionError('pattern "%s" not found in "%s"'
- % (needle_re, haystack))
-
- def catchLogEvents(self):
- """Any log events that are triggered get written to self.log_stream"""
- logger = logging.getLogger('lp-loggerhead')
- logger.setLevel(logging.DEBUG)
- self.log_stream = cStringIO.StringIO()
- handler = logging.StreamHandler(self.log_stream)
- handler.setLevel(logging.DEBUG)
- logger.addHandler(handler)
- self.addCleanup(logger.removeHandler, handler)
-
- def runtime_failing_app(self, environ, start_response):
- if False:
- yield None
- raise RuntimeError('just a generic runtime error.')
-
- def socket_failing_app(self, environ, start_response):
- if False:
- yield None
- raise socket.error(errno.EPIPE, 'Connection closed')
-
- def logging_start_response(self, status, response_headers, exc_info=None):
- self._response_chunks = []
- def _write(chunk):
- self._response_chunks.append(chunk)
- self.start_response_called = True
- return _write
-
- def success_app(self, environ, start_response):
- writer = start_response('200 OK', {})
- writer('Successfull\n')
- return []
-
- def failing_start_response(self, status, response_headers, exc_info=None):
- def fail_write(chunk):
- raise socket.error(errno.EPIPE, 'Connection closed')
- self.start_response_called = True
- return fail_write
-
- def multi_yielding_app(self, environ, start_response):
- writer = start_response('200 OK', {})
- yield 'content\n'
- yield 'I want\n'
- yield 'to give to the user\n'
-
- def no_body_app(self, environ, start_response):
- writer = start_response('200 OK', {})
- return []
-
- def _get_default_environ(self):
- return {'wsgi.version': (1, 0),
- 'wsgi.url_scheme': 'http',
- 'PATH_INFO': '/test/path',
- 'REQUEST_METHOD': 'GET',
- 'SERVER_NAME': 'localhost',
- 'SERVER_PORT': '8080',
- }
-
- def wrap_and_run(self, app, failing_write=False):
- app = oops_middleware(app)
- # Just random env data, rather than setting up a whole wsgi stack just
- # to pass in values for this dict
- environ = self._get_default_environ()
- if failing_write:
- result = list(app(environ, self.failing_start_response))
- else:
- result = list(app(environ, self.logging_start_response))
- return result
-
- def test_exception_triggers_oops(self):
- res = self.wrap_and_run(self.runtime_failing_app)
- # After the exception was raised, we should also have gotten an oops
- # event
- self.assertEqual(1, len(self.oopses))
- oops = self.oopses[0]
- self.assertEqual('RuntimeError', oops['type'])
- # runtime_failing_app doesn't call start_response, but oops_middleware
- # does because it tries to send the OOPS information to the user.
- self.assertTrue(self.start_response_called)
- self.assertEqual(_oops_html_template % {'oopsid': oops['id']},
- ''.join(self._response_chunks))
-
- def test_ignores_socket_exceptions(self):
- self.catchLogEvents()
- res = self.wrap_and_run(self.socket_failing_app)
- self.assertEqual(0, len(self.oopses))
- self.assertContainsRe(self.log_stream.getvalue(),
- 'Caught socket exception from <unknown>:.*Connection closed')
- # start_response doesn't get called because the app fails first,
- # and oops_middleware knows not to do anything with a closed socket.
- self.assertFalse(self.start_response_called)
-
- def test_ignores_writer_failures(self):
- self.catchLogEvents()
- res = self.wrap_and_run(self.success_app, failing_write=True)
- self.assertEqual(0, len(self.oopses))
- self.assertContainsRe(self.log_stream.getvalue(),
- 'Caught socket exception from <unknown>:.*Connection closed')
- # success_app calls start_response, so this should get passed on.
- self.assertTrue(self.start_response_called)
-
- def test_stopping_early_no_oops(self):
- # See bug #726985.
- # If content is being streamed, and the pipe closes, we'll get a
- # 'GeneratorExit', because it is closed before finishing. This doesn't
- # need to become an OOPS.
- self.catchLogEvents()
- app = oops_middleware(self.multi_yielding_app)
- environ = self._get_default_environ()
- result = app(environ, self.logging_start_response)
- self.assertEqual('content\n', result.next())
- # At this point, we intentionally kill the app and the response, so
- # that they will get GeneratorExit
- del app, result
- self.assertEqual([], self.oopses)
- self.assertContainsRe(self.log_stream.getvalue(),
- 'Caught GeneratorExit from <unknown>')
- # Body content was yielded, we must have called start_response
- self.assertTrue(self.start_response_called)
-
- def test_no_body_calls_start_response(self):
- # See bug #732481, even if we don't have a body, if we have headers to
- # send, we must call start_response
- result = self.wrap_and_run(self.no_body_app)
- self.assertEqual([], result)
- self.assertTrue(self.start_response_called)
- # Output content is empty because of no_body_app
- self.assertEqual('', ''.join(self._response_chunks))
=== modified file 'setup.py'
--- setup.py 2011-08-15 09:21:48 +0000
+++ setup.py 2011-08-18 03:43:26 +0000
@@ -58,6 +58,7 @@
'oauth',
'oops',
'oops_datedir_repo',
+ 'oops_wsgi',
'paramiko',
'psycopg2',
'python-memcached',
=== modified file 'versions.cfg'
--- versions.cfg 2011-08-17 08:09:43 +0000
+++ versions.cfg 2011-08-18 03:43:26 +0000
@@ -51,6 +51,7 @@
oauth = 1.0
oops = 0.0.6
oops-datedir-repo = 0.0.3
+oops-wsgi = 0.0.1
paramiko = 1.7.4
Paste = 1.7.2
PasteDeploy = 1.3.3
