Merge lp:~salgado/storm/fix-wsgi into lp:storm

[Guilherme Salgado]

Fix wsgi.make_app() to not make assumptions about how to consume data from
whatever is returned by its inner app() call

We cannot use storm.wsgi.make_app() in U1 because it materializes the
IBodyProducer we return, thus defeating its purpose.

[Thomas Herve (therve)]

Maybe I miss something, but returning a IBodyProducer is not WSGI? WSGI expects an iterable, and that's what's done here.

[Sidnei da Silva (sidnei)]

Correct. We special-case an IBodyProducer return value in U1's twisted wsgi container, so that when returning large files we let the reactor consume that asynchronously instead of running the iterable in a thread. Yes, it's not pure WSGI per the spec.

The change here though should be innocuous as long as you do not leak threads. We discussed this with Robert and he thinks it's fine enough, although perhaps we could use a weakref for extra safety.

[Thomas Herve (therve)]

Please fix the comment to mention some you're not really using it in a WSGI app. +1!

review: Approve

[Sidnei da Silva (sidnei)]

+1 with using a weakref for extra safety.

review: Approve

[lp:~salgado/storm/fix-wsgi]

449. By Guilherme Salgado on 2012-06-04

Fix the comment in wsgi.make_app() and use a wekreference for timeline_map.timeline

[Guilherme Salgado (salgado)]

I've changed the comment and am now using a weakref for .timeline. It'd be great if you guys could have another quick look

[Sidnei da Silva (sidnei)]

Awesome. +1!

=== modified file 'storm/wsgi.py'
--- storm/wsgi.py	2011-09-19 11:55:50 +0000
+++ storm/wsgi.py	2012-06-04 14:20:48 +0000
@@ -21,8 +21,8 @@
 
 """Glue to wire a storm timeline tracer up to a WSGI app."""
 
-import functools
 import threading
+import weakref
 
 __all__ = ['make_app']
 
@@ -48,11 +48,18 @@
     timeline_map = threading.local()
     def wrapper(environ, start_response):
         timeline = environ.get('timeline.timeline')
-        timeline_map.timeline = timeline
-        try:
-            gen = app(environ, start_response)
-            for bytes in gen:
-                yield bytes
-        finally:
-            del timeline_map.timeline
-    return wrapper, functools.partial(getattr, timeline_map, 'timeline', None)
+        timeline_map.timeline = None
+        if timeline is not None:
+            timeline_map.timeline = weakref.ref(timeline)
+        # We could clean up timeline_map.timeline after we're done with the
+        # request, but for that we'd have to consume all the data from the
+        # underlying app and it wouldn't play well with some non-standard
+        # tricks (e.g. let the reactor consume IBodyProducers asynchronously
+        # when returning large files) that some people may want to do.
+        return app(environ, start_response)
+
+    def get_timeline():
+        timeline = getattr(timeline_map, 'timeline', None)
+        if timeline is not None:
+            return timeline()
+    return wrapper, get_timeline

=== modified file 'tests/wsgi.py'
--- tests/wsgi.py	2011-09-19 11:55:50 +0000
+++ tests/wsgi.py	2012-06-04 14:20:48 +0000
@@ -37,41 +37,29 @@
     def test_find_timeline_set_in_environ(self):
         # If a timeline object is known, find_timeline finds it:
         app, find_timeline = make_app(self.stub_app)
-        timeline = object()
+        timeline = FakeTimeline()
         self.in_request = lambda:self.assertEqual(timeline, find_timeline())
         list(app({'timeline.timeline': timeline}, self.stub_start_response))
-        # Having left the request, no timeline is known:
-        self.assertEqual(None, find_timeline())
 
     def test_find_timeline_set_in_environ_during_generator(self):
         # If a timeline object is known, find_timeline finds it:
         app, find_timeline = make_app(self.stub_app)
-        timeline = object()
+        timeline = FakeTimeline()
         self.in_generator = lambda:self.assertEqual(timeline, find_timeline())
         list(app({'timeline.timeline': timeline}, self.stub_start_response))
-        # Having left the request, no timeline is known:
-        self.assertEqual(None, find_timeline())
-
-    def raiser(self):
-        raise ValueError('foo')
-
-    def test_find_timeline_exception_in_app_still_gets_cleared(self):
-        self.in_request = self.raiser
-        app, find_timeline = make_app(self.stub_app)
-        timeline = object()
-        self.assertRaises(
-            ValueError, lambda: list(app({'timeline.timeline': timeline},
-            self.stub_start_response)))
-        self.assertEqual(None, find_timeline())
-
-    def test_find_timeline_exception_in_generator_still_gets_cleared(self):
-        self.in_generator = self.raiser
-        app, find_timeline = make_app(self.stub_app)
-        timeline = object()
-        self.assertRaises(
-            ValueError, lambda: list(app({'timeline.timeline': timeline},
-            self.stub_start_response)))
-        self.assertEqual(None, find_timeline())
+
+    def test_timeline_is_replaced_in_subsequent_request(self):
+        app, find_timeline = make_app(self.stub_app)
+        timeline = FakeTimeline()
+        self.in_request = lambda:self.assertEqual(timeline, find_timeline())
+        list(app({'timeline.timeline': timeline}, self.stub_start_response))
+
+        # Having left the request, the timeline is left behind...
+        self.assertEqual(timeline, find_timeline())
+        # ... but only until the next request comes through.
+        timeline2 = FakeTimeline()
+        self.in_request = lambda:self.assertEqual(timeline2, find_timeline())
+        list(app({'timeline.timeline': timeline2}, self.stub_start_response))
 
     def test_lookups_are_threaded(self):
         # with two threads in a request at once, each only sees their own
@@ -81,7 +69,7 @@
         sync = threading.Condition()
         waiting = []
         def check_timeline():
-            timeline = object()
+            timeline = FakeTimeline()
             def start_response(status, headers):
                 # Block on the condition, so all test threads are in
                 # start_response when the test resumes.
@@ -115,3 +103,11 @@
         if errors.qsize():
             found_timeline, timeline = errors.get(False)
             self.assertEqual(timeline, found_timeline)
+
+
+class FakeTimeline(object):
+    """A fake Timeline.
+
+    We need this because we can't use plain object instances as they can't be
+    weakreferenced.
+    """