Merge lp:~james-w/python-timeline-django/clean-env into lp:python-timeline-django

[James Westby]

Be more careful about leaking objects.

1. Register signals with a dispatch_uid so that if register is called
   twice there aren't double receivers. There is a very slim chance that
   someone could two want two sets of receivers with timeline_factories
   that get different timelines. In that case this should be rewritten
   to not use dispatch_uid and offer a disconnect method that can be
   used in tests etc.

2. Re-set the environ stored in the thread local after the wsgi response
   is delivered. In a case where some requests go through wsgi and some
   don't (e.g. tests) the environ would leak in to those that don't use
   wsgi. This could lead to an ever-growing timeline.

Hi,

This fixes the problems that timeline was causing in the sca test suite.

Thanks,

James

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

[lp:~james-w/python-timeline-django/clean-env]

24. By James Westby on 2014-08-29

Add missing space, thanks Natalia.

=== modified file 'timeline_django/hooks.py'
--- timeline_django/hooks.py	2014-03-18 16:47:46 +0000
+++ timeline_django/hooks.py	2014-08-29 14:46:17 +0000
@@ -14,7 +14,6 @@
 # GNU Lesser General Public License version 3 (see the file LICENSE).
 
 from datetime import timedelta
-import threading
 
 import django.core.signals
 import django.db.backends.signals
@@ -87,9 +86,15 @@
 
     def connect_to_signals(self):
         """Connect the callbacks to their corresponding signals."""
-        django.core.signals.request_started.connect(self.request_started, weak=False)
-        django.core.signals.request_finished.connect(self.request_finished, weak=False)
-        django.core.signals.got_request_exception.connect(self.got_request_exception, weak=False)
-        django.db.backends.signals.connection_created.connect(self.connection_created, weak=False)
-        signals.wsgi_request_started.connect(self.wsgi_request_started, weak=False)
-        signals.wsgi_response_started.connect(self.wsgi_response_started, weak=False)
+        django.core.signals.request_started.connect(
+            self.request_started, weak=False, dispatch_uid="timeline_django")
+        django.core.signals.request_finished.connect(
+            self.request_finished, weak=False, dispatch_uid="timeline_django")
+        django.core.signals.got_request_exception.connect(
+            self.got_request_exception, weak=False, dispatch_uid="timeline_django")
+        django.db.backends.signals.connection_created.connect(
+            self.connection_created, weak=False, dispatch_uid="timeline_django")
+        signals.wsgi_request_started.connect(
+            self.wsgi_request_started, weak=False, dispatch_uid="timeline_django")
+        signals.wsgi_response_started.connect(
+            self.wsgi_response_started, weak=False, dispatch_uid="timeline_django")

=== modified file 'timeline_django/tests/test_hooks.py'
--- timeline_django/tests/test_hooks.py	2014-03-18 16:47:46 +0000
+++ timeline_django/tests/test_hooks.py	2014-08-29 14:46:17 +0000
@@ -252,28 +252,34 @@
         signal, receiver = self.register_signal(django.core.signals, 'request_started')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.request_started, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])
 
     def test_register_signals_registers_request_finished(self):
         signal, receiver = self.register_signal(django.core.signals, 'request_finished')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.request_finished, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])
 
     def test_register_signals_registers_got_request_exception(self):
         signal, receiver = self.register_signal(django.core.signals, 'got_request_exception')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.got_request_exception, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])
 
     def test_register_signals_registers_connection_created(self):
         signal, receiver = self.register_signal(django.db.backends.signals, 'connection_created')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.connection_created, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])
 
     def test_register_signals_registers_wsgi_request_started(self):
         signal, receiver = self.register_signal(signals, 'wsgi_request_started')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.wsgi_request_started, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])
 
     def test_register_signals_registers_wsgi_response_started(self):
         signal, receiver = self.register_signal(signals, 'wsgi_response_started')
         self.assertEqual(1, len(signal.receivers))
         self.assertEqual(receiver.wsgi_response_started, signal.receivers[0][1])
+        self.assertEqual("timeline_django", signal.receivers[0][0][0])

=== modified file 'timeline_django/tests/test_wsgi.py'
--- timeline_django/tests/test_wsgi.py	2013-09-03 00:25:21 +0000
+++ timeline_django/tests/test_wsgi.py	2014-08-29 14:46:17 +0000
@@ -37,12 +37,22 @@
 
     def test_wsgi_sets_timeline(self):
         environ = {}
-        start_response = lambda status, headers:None
+        start_response = lambda status, headers: None
+        def do_check(environ, start_response):
+            self.assertThat(wsgi.get_environ(), Is(environ))
+            yield "foo"
+        app = wsgi.make_app(do_check)
+        out = app(environ, start_response)
+        self.assertEqual(["foo"], list(out))
+
+    def test_wsgi_unsets_timeline(self):
+        environ = {}
+        start_response = lambda status, headers: None
         app = lambda environ, start_response: ["foo"]
         app = wsgi.make_app(app)
         out = app(environ, start_response)
-        self.assertEqual(["foo"], out)
-        self.assertThat(wsgi.get_environ(), Is(environ))
+        self.assertEqual(["foo"], list(out))
+        self.assertThat(wsgi.get_environ(), Is(None))
 
     def patch_signal(self, name):
         signal = Signal()
@@ -93,3 +103,38 @@
         wrapped_app(environ, None)
         # Check that the saved environ matches the one passed to the app
         self.assertEqual([environ], called_environs)
+
+
+class GeneratorTrackerTests(TestCase):
+
+    def test_consumes_generator(self):
+        app = wsgi.generator_tracker(lambda: True, ["foo"])
+        self.assertEqual(["foo"], list(app))
+
+    def test_calls_on_finish(self):
+        calls = []
+        def generator():
+            calls.append("consume")
+            yield "foo"
+        def on_finish():
+            calls.append("on_finish")
+        app = wsgi.generator_tracker(on_finish, generator())
+        self.assertEqual(["foo"], list(app))
+        self.assertEqual(["consume", "on_finish"], calls)
+
+    def test_calls_close(self):
+        calls = []
+        class Body(object):
+
+            def __iter__(self):
+                calls.append("consume")
+                yield "foo"
+
+            def close(self):
+                calls.append("close")
+
+        def on_finish():
+            calls.append("on_finish")
+        app = wsgi.generator_tracker(on_finish, Body())
+        self.assertEqual(["foo"], list(app))
+        self.assertEqual(["consume", "close", "on_finish"], calls)

=== modified file 'timeline_django/wsgi.py'
--- timeline_django/wsgi.py	2013-09-03 00:25:21 +0000
+++ timeline_django/wsgi.py	2014-08-29 14:46:17 +0000
@@ -52,5 +52,17 @@
             signals.wsgi_response_started.send(sender=save_environ,
                                                environ=environ)
             return start_response(*args, **kwargs)
-        return application(environ, signalled_start_response)
+        def remove_env():
+            _thread_local.environ = None
+        return generator_tracker(remove_env, application(environ, signalled_start_response))
     return save_environ
+
+
+def generator_tracker(on_finish, app_body):
+    try:
+        for bytes in app_body:
+            yield bytes
+    finally:
+        if hasattr(app_body, 'close'):
+            app_body.close()
+        on_finish()