launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05419
[Merge] lp:~lifeless/python-oops-wsgi/0.0.6 into lp:python-oops-wsgi
Robert Collins has proposed merging lp:~lifeless/python-oops-wsgi/0.0.6 into lp:python-oops-wsgi.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/python-oops-wsgi/0.0.6/+merge/81230
This fixes a small but critical bug (0-length iterators didn't call start_response) and permits arbitrary customisation of the tracking of the response body. NEWS says it all.
--
https://code.launchpad.net/~lifeless/python-oops-wsgi/0.0.6/+merge/81230
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-wsgi/0.0.6 into lp:python-oops-wsgi.
=== added file '.testr.conf'
--- .testr.conf 1970-01-01 00:00:00 +0000
+++ .testr.conf 2011-11-04 04:01:23 +0000
@@ -0,0 +1,4 @@
+[DEFAULT]
+test_command=PYTHONPATH=. bin/py -m subunit.run $LISTOPT $IDOPTION oops_wsgi.tests.test_suite
+test_id_option=--load-list $IDFILE
+test_list_option=--list
=== modified file 'NEWS'
--- NEWS 2011-09-23 12:02:16 +0000
+++ NEWS 2011-11-04 04:01:23 +0000
@@ -6,6 +6,20 @@
NEXT
----
+0.0.6
+-----
+
+* Applications which return [] as their body, and do not return a generator,
+ now work correctly. This was broken if the iterator loop never executed,
+ because middleware cannot assume that calling app(environ, start_response)
+ will cause start_response to be triggered. (Robert Collins)
+
+* The wrapping of the application body is now customisable by supplying a
+ tracker factory to make_app. This permits special handling of bodies (e.g. by
+ taking them out of threads and putting them into an event loop, once the type
+ is known). See oops-twisted in the near future for an example of such a thing.
+ (Robert Collins)
+
0.0.4
-----
=== modified file 'README'
--- README 2011-09-23 12:02:16 +0000
+++ README 2011-11-04 04:01:23 +0000
@@ -113,3 +113,7 @@
For instance::
$ bin/py -m testtools.run oops_wsgi.tests.test_suite
+
+If you have testrepository you can run the tests with that::
+
+ $ testr run
=== modified file 'oops_wsgi/__init__.py'
--- oops_wsgi/__init__.py 2011-09-23 12:02:16 +0000
+++ oops_wsgi/__init__.py 2011-11-04 04:01:23 +0000
@@ -92,6 +92,9 @@
If a timeline is present in the WSGI environ (as 'timeline.timeline') it is
automatically captured to the oops context when generating an OOPS. See the
oops-timeline module for hooks to use this.
+
+`pydoc oops_wsgi.make_app` describes the entire capabilities of the
+middleware.
"""
@@ -106,7 +109,7 @@
# established at this point, and setup.py will use a version of next-$(revno).
# If the releaselevel is 'final', then the tarball will be major.minor.micro.
# Otherwise it is major.minor.micro~$(revno).
-__version__ = (0, 0, 5, 'beta', 0)
+__version__ = (0, 0, 6, 'beta', 0)
__all__ = [
'install_hooks',
=== modified file 'oops_wsgi/middleware.py'
--- oops_wsgi/middleware.py 2011-09-20 03:03:01 +0000
+++ oops_wsgi/middleware.py 2011-11-04 04:01:23 +0000
@@ -25,6 +25,7 @@
__all__ = [
'default_map_environ',
+ 'generator_tracker',
'make_app',
]
@@ -35,7 +36,7 @@
<h1>Oops!</h1>
<p>Something broke while generating the page.
Please try again in a few minutes, and if the problem persists file
-a bug or contact customer support. PLease quote OOPS-ID
+a bug or contact customer support. Please quote OOPS-ID
<strong>%(id)s</strong>
</p></body></html>'''
@@ -48,7 +49,7 @@
def make_app(app, config, template=default_error_template,
content_type='text/html', error_render=None, oops_on_status=None,
- map_environ=None):
+ map_environ=None, tracker=None):
"""Construct a middleware around app that will forward errors via config.
Any errors encountered by the app will be forwarded to config and an error
@@ -81,6 +82,11 @@
present map into the OOPS context when generating an OOPS. The value of
the key determines the name given in the OOPS context. If None is passed
the default_map_environ is used. Pass {} in to entirely disable mapping.
+ :param tracker: A factory function to create a tracker. Trackers are used
+ to allow variations on the WSGI environment to still use oops_wsgi.
+ See generator_tracker for the reference tracker used in regular WSGI
+ environments. generator_tracker is used by default or when
+ tracker=None.
:return: A WSGI app.
"""
def oops_middleware(environ, start_response):
@@ -143,35 +149,77 @@
state['response'] = (status, headers)
return oops_write
try:
- iterator = iter(app(environ, oops_start_response))
- for bytes in iterator:
+ def ensure_start_response():
if 'write' not in state:
status, headers = state.pop('response')
# Signal that we have called start_response
state['write'] = start_response(status, headers)
- yield bytes
+ def on_exception(exc_info):
+ report = config.create(make_context(exc_info=exc_info))
+ ids = config.publish(report)
+ if not ids or 'write' in state:
+ # No OOPS generated, no oops publisher, or we have already
+ # transmitted the wrapped apps headers - either way we can't
+ # replace the content with a clean error, so let the wsgi
+ # server figure it out.
+ raise
+ headers = [('Content-Type', content_type)]
+ headers.append(('X-Oops-Id', str(report['id'])))
+ start_response(
+ '500 Internal Server Error', headers, exc_info=exc_info)
+ del exc_info
+ if error_render is not None:
+ return error_render(report)
+ else:
+ return template % report
+ if tracker is None:
+ tracker_factory = generator_tracker
+ else:
+ tracker_factory = tracker
+ return tracker_factory(
+ ensure_start_response, ensure_start_response, on_exception,
+ app(environ, oops_start_response))
except socket.error:
raise
- except GeneratorExit:
- raise
except Exception:
exc_info = sys.exc_info()
- report = config.create(make_context(exc_info=exc_info))
- ids = config.publish(report)
- if not ids or 'write' in state:
- # No OOPS generated, no oops publisher, or we have already
- # transmitted the wrapped apps headers - either way we can't
- # replace the content with a clean error, so let the wsgi
- # server figure it out.
- raise
- headers = [('Content-Type', content_type)]
- headers.append(('X-Oops-Id', str(report['id'])))
- start_response(
- '500 Internal Server Error', headers, exc_info=exc_info)
- del exc_info
- if error_render is not None:
- yield error_render(report)
- else:
- yield template % report
+ return [on_exception(exc_info)]
return oops_middleware
+
+
+def generator_tracker(on_first_bytes, on_finish, on_error, app_body):
+ """A wrapper for generators that calls the OOPS hooks as needed.
+
+ :param on_first_bytes: Called as on_first_bytes() when the first bytes from
+ the app body are available but before they are yielded.
+ :param on_finish: Called as on_finish() when the app body is fully
+ consumed.
+ :param on_error: Called as on_error(sys.exc_info()) if a handleable error
+ has occured while consuming the generator. Errors like GeneratorExit
+ are not handleable.
+ :param app_body: The iterable body for the WSGI app. This may be a simple
+ list or a generator - it is merely known to meet the iterator protocol.
+ """
+ try:
+ called_first = False
+ for bytes in app_body:
+ if not called_first:
+ called_first = True
+ on_first_bytes()
+ yield bytes
+ on_finish()
+ except socket.error:
+ # start_response, which iteration can trigger a call into, may raise
+ # socket.error when writing if the client has disconnected: thats not
+ # an OOPS condition. This does potentially mask socket.error issues in
+ # the appserver code, so we may want to change this to callback to
+ # determine if start_response has been called upstream, and if so, to
+ # still generate an OOPS.
+ raise
+ except GeneratorExit:
+ # Python 2.4
+ raise
+ except Exception:
+ exc_info = sys.exc_info()
+ yield on_error(exc_info)
=== modified file 'oops_wsgi/tests/test_middleware.py'
--- oops_wsgi/tests/test_middleware.py 2011-09-20 03:03:01 +0000
+++ oops_wsgi/tests/test_middleware.py 2011-11-04 04:01:23 +0000
@@ -38,6 +38,7 @@
)
from oops_wsgi import make_app
+from oops_wsgi.middleware import generator_tracker
class MatchesOOPS:
@@ -154,6 +155,19 @@
('200 OK', [('Content-Type', 'text/html')]),
], self.calls)
+ def test_empty_body(self):
+ def inner(environ, start_response):
+ start_response('200 OK', [('Content-Type', 'text/html')])
+ self.calls.append('inner')
+ return []
+ self.wrap_and_run(inner, Config())
+ self.assertEqual([
+ # the start_reponse gets buffered in case the body production
+ # fails.
+ 'inner',
+ ('200 OK', [('Content-Type', 'text/html')]),
+ ], self.calls)
+
def test_unpublished_exception_raises(self):
def inner(environ, start_response):
raise ValueError('foo')
@@ -291,7 +305,7 @@
<h1>Oops!</h1>
<p>Something broke while generating the page.
Please try again in a few minutes, and if the problem persists file
- a bug or contact customer support. PLease quote OOPS-ID
+ a bug or contact customer support. Please quote OOPS-ID
<strong>...</strong>
</p></body></html>
"""),ELLIPSIS))
@@ -484,3 +498,81 @@
config = self.config_for_oopsing()
body = self.wrap_and_run(inner, config)
self.assertEqual('success', body)
+
+ def test_custom_tracker(self):
+ def myapp(environ, start_response):
+ start_response('200 OK', [])
+ yield 'success'
+ def mytracker(on_first_bytes, on_finish, on_error, body):
+ return 'tracker'
+ app = make_app(myapp, Config(), tracker=mytracker)
+ self.assertEqual('tracker', app({}, lambda status, headers:None))
+
+
+class TestGeneratorTracker(TestCase):
+
+ def setUp(self):
+ super(TestGeneratorTracker, self).setUp()
+ self.calls = []
+
+ def on_first_bytes(self):
+ self.calls.append('on_first_bytes')
+
+ def on_finish(self):
+ self.calls.append('on_finish')
+
+ def on_exception_ok(self, exc_info):
+ self.calls.append('on exception %s' % (exc_info[1],))
+ return 'error page'
+
+ def on_exception_fail(self, exc_info):
+ self.calls.append('on exception %s' % (exc_info[1],))
+ raise ValueError('fail')
+
+ def test_constructor(self):
+ tracker = generator_tracker(None, None, None, [])
+
+ def test_call_order_empty(self):
+ tracker = generator_tracker(
+ self.on_first_bytes, self.on_finish, None, [])
+ self.assertEqual([], list(tracker))
+ self.assertEqual(['on_finish'], self.calls)
+
+ def test_call_order_one_item(self):
+ tracker = generator_tracker(
+ self.on_first_bytes, self.on_finish, None, ['foo'])
+ for result in tracker:
+ self.calls.append(result)
+ self.assertEqual(['on_first_bytes', 'foo', 'on_finish'], self.calls)
+
+ def test_call_order_two_items(self):
+ def on_first_bytes():
+ self.calls.append('on_first_bytes')
+ tracker = generator_tracker(
+ self.on_first_bytes, self.on_finish, None, ['foo', 'bar'])
+ for result in tracker:
+ self.calls.append(result)
+ self.assertEqual(
+ ['on_first_bytes', 'foo', 'bar', 'on_finish'], self.calls)
+
+ def test_on_exception_iter(self):
+ def failing_iter():
+ raise ValueError('foo')
+ yield ''
+ tracker = generator_tracker(
+ self.on_first_bytes, self.on_finish, self.on_exception_ok,
+ failing_iter())
+ for result in tracker:
+ self.calls.append(result)
+ self.assertEqual(['on exception foo', 'error page'], self.calls)
+
+ def test_on_exception_exceptions_propogate(self):
+ def failing_iter():
+ raise ValueError('foo')
+ yield ''
+ tracker = generator_tracker(
+ self.on_first_bytes, self.on_finish, self.on_exception_fail,
+ failing_iter())
+ self.assertThat(lambda:tracker.next(), raises(ValueError('fail')))
+ self.assertEqual(['on exception foo'], self.calls)
+
=== modified file 'setup.py'
--- setup.py 2011-09-23 12:02:16 +0000
+++ setup.py 2011-11-04 04:01:23 +0000
@@ -23,7 +23,7 @@
os.path.join(os.path.dirname(__file__), 'README'), 'rb').read()
setup(name="oops_wsgi",
- version="0.0.5",
+ version="0.0.6",
description=\
"OOPS wsgi middleware.",
long_description=description,
