move p.response.temporary_response context manager to p.util.hide_attrs

5 files changed, 110 insertions, 82 deletions

diff --git a/pyramid/renderers.py b/pyramid/renderers.py
index 456b16c82..bcbcbb0aa 100644
--- a/pyramid/renderers.py
+++ b/pyramid/renderers.py
@@ -1,4 +1,3 @@
-import contextlib
 import json
 import os
 import re
@@ -30,6 +29,7 @@ from pyramid.path import caller_package
 
 from pyramid.response import _get_response_factory
 from pyramid.threadlocal import get_current_registry
+from pyramid.util import hide_attrs
 
 # API
 
@@ -77,7 +77,7 @@ def render(renderer_name, value, request=None, package=None):
     helper = RendererHelper(name=renderer_name, package=package,
                             registry=registry)
 
-    with temporary_response(request):
+    with hide_attrs(request, 'response'):
         result = helper.render(value, None, request=request)
 
     return result
@@ -138,30 +138,13 @@ def render_to_response(renderer_name,
     helper = RendererHelper(name=renderer_name, package=package,
                             registry=registry)
 
-    with temporary_response(request):
+    with hide_attrs(request, 'response'):
         if response is not None:
             request.response = response
         result = helper.render_to_response(value, None, request=request)
 
     return result
 
-_marker = object()
-
-@contextlib.contextmanager
-def temporary_response(request):
-    """
-    Temporarily delete request.response and restore it afterward.
-    """
-    attrs = request.__dict__ if request is not None else {}
-    saved_response = attrs.pop('response', _marker)
-    try:
-        yield
-    finally:
-        if saved_response is not _marker:
-            attrs['response'] = saved_response
-        elif 'response' in attrs:
-            del attrs['response']
-
 def get_renderer(renderer_name, package=None):
     """ Return the renderer object for the renderer ``renderer_name``.
 
diff --git a/pyramid/tests/test_renderers.py b/pyramid/tests/test_renderers.py
index 2458ea830..65bfa5582 100644
--- a/pyramid/tests/test_renderers.py
+++ b/pyramid/tests/test_renderers.py
@@ -592,48 +592,6 @@ class Test_render_to_response(unittest.TestCase):
         self.assertEqual(result.body, b'{"a": 1}')
         self.assertFalse('response' in request.__dict__)
 
-class Test_temporary_response(unittest.TestCase):
-    def _callFUT(self, request):
-        from pyramid.renderers import temporary_response
-        return temporary_response(request)
-
-    def test_restores_response(self):
-        request = testing.DummyRequest()
-        orig_response = request.response
-        with self._callFUT(request):
-            request.response = object()
-        self.assertEqual(request.response, orig_response)
-
-    def test_restores_response_on_exception(self):
-        request = testing.DummyRequest()
-        orig_response = request.response
-        try:
-            with self._callFUT(request):
-                request.response = object()
-                raise RuntimeError()
-        except RuntimeError:
-            self.assertEqual(request.response, orig_response)
-        else:                   # pragma: no cover
-            self.fail("RuntimeError not raised")
-
-    def test_restores_response_to_none(self):
-        request = testing.DummyRequest(response=None)
-        with self._callFUT(request):
-            request.response = object()
-        self.assertEqual(request.response, None)
-
-    def test_deletes_response(self):
-        request = testing.DummyRequest()
-        with self._callFUT(request):
-            request.response = object()
-        self.assertTrue('response' not in request.__dict__)
-
-    def test_does_not_delete_response_if_no_response_to_delete(self):
-        request = testing.DummyRequest()
-        with self._callFUT(request):
-            pass
-        self.assertTrue('response' not in request.__dict__)
-
 class Test_get_renderer(unittest.TestCase):
     def setUp(self):
         self.config = testing.setUp()
diff --git a/pyramid/tests/test_util.py b/pyramid/tests/test_util.py
index 0be99e949..c606a4b6b 100644
--- a/pyramid/tests/test_util.py
+++ b/pyramid/tests/test_util.py
@@ -794,6 +794,63 @@ class TestCallableName(unittest.TestCase):
         self.assertRaises(ConfigurationError, get_bad_name)
 
 
+class Test_hide_attrs(unittest.TestCase):
+    def _callFUT(self, obj, *attrs):
+        from pyramid.util import hide_attrs
+        return hide_attrs(obj, *attrs)
+
+    def _makeDummy(self):
+        from pyramid.decorator import reify
+        class Dummy(object):
+            x = 1
+
+            @reify
+            def foo(self):
+                return self.x
+        return Dummy()
+
+    def test_restores_attrs(self):
+        obj = self._makeDummy()
+        obj.bar = 'asdf'
+        orig_foo = obj.foo
+        with self._callFUT(obj, 'foo', 'bar'):
+            obj.foo = object()
+            obj.bar = 'nope'
+        self.assertEqual(obj.foo, orig_foo)
+        self.assertEqual(obj.bar, 'asdf')
+
+    def test_restores_attrs_on_exception(self):
+        obj = self._makeDummy()
+        orig_foo = obj.foo
+        try:
+            with self._callFUT(obj, 'foo'):
+                obj.foo = object()
+                raise RuntimeError()
+        except RuntimeError:
+            self.assertEqual(obj.foo, orig_foo)
+        else:                   # pragma: no cover
+            self.fail("RuntimeError not raised")
+
+    def test_restores_attrs_to_none(self):
+        obj = self._makeDummy()
+        obj.foo = None
+        with self._callFUT(obj, 'foo'):
+            obj.foo = object()
+        self.assertEqual(obj.foo, None)
+
+    def test_deletes_attrs(self):
+        obj = self._makeDummy()
+        with self._callFUT(obj, 'foo'):
+            obj.foo = object()
+        self.assertTrue('foo' not in obj.__dict__)
+
+    def test_does_not_delete_attr_if_no_attr_to_delete(self):
+        obj = self._makeDummy()
+        with self._callFUT(obj, 'foo'):
+            pass
+        self.assertTrue('foo' not in obj.__dict__)
+
+
 def dummyfunc(): pass
 
 
diff --git a/pyramid/util.py b/pyramid/util.py
index 0a73cedaf..e1113e0ec 100644
--- a/pyramid/util.py
+++ b/pyramid/util.py
@@ -1,3 +1,4 @@
+import contextlib
 import functools
 try:
     # py2.7.7+ and py3.3+ have native comparison support
@@ -591,3 +592,22 @@ def get_callable_name(name):
             'used on __name__ of the method'
         )
         raise ConfigurationError(msg % name)
+
+@contextlib.contextmanager
+def hide_attrs(obj, *attrs):
+    """
+    Temporarily delete object attrs and restore afterward.
+    """
+    obj_vals = obj.__dict__ if obj is not None else {}
+    saved_vals = {}
+    for name in attrs:
+        saved_vals[name] = obj_vals.pop(name, _marker)
+    try:
+        yield
+    finally:
+        for name in attrs:
+            saved_val = saved_vals[name]
+            if saved_val is not _marker:
+                obj_vals[name] = saved_val
+            elif name in obj_vals:
+                del obj_vals[name]
diff --git a/pyramid/view.py b/pyramid/view.py
index 1f20622dc..2966b0a50 100644
--- a/pyramid/view.py
+++ b/pyramid/view.py
@@ -25,6 +25,7 @@ from pyramid.httpexceptions import (
     )
 
 from pyramid.threadlocal import get_current_registry
+from pyramid.util import hide_attrs
 
 _marker = object()
 
@@ -600,23 +601,32 @@ class ViewMethodsMixin(object):
         attrs = request.__dict__
         context_iface = providedBy(exc_info[0])
         view_name = attrs.get('view_name', '')
-        # probably need something like "with temporarily_munged_request(req)"
-        # here, which adds exception and exc_info as request attrs, and
-        # removes response object temporarily (as per the excview tween)
-        attrs['exception'] = exc_info[0]
-        attrs['exc_info'] = exc_info
-        # we use .get instead of .__getitem__ below due to
-        # https://github.com/Pylons/pyramid/issues/700
-        request_iface = attrs.get('request_iface', IRequest)
-        response = _call_view(
-            registry,
-            request,
-            exc_info[0],
-            context_iface,
-            view_name,
-            view_types=None,
-            view_classifier=IExceptionViewClassifier,
-            secure=secure,
-            request_iface=request_iface.combined,
-            )
-        return response
+
+        # WARNING: do not assign the result of sys.exc_info() to a local
+        # var here, doing so will cause a leak.  We used to actually
+        # explicitly delete both "exception" and "exc_info" from ``attrs``
+        # in a ``finally:`` clause below, but now we do not because these
+        # attributes are useful to upstream tweens.  This actually still
+        # apparently causes a reference cycle, but it is broken
+        # successfully by the garbage collector (see
+        # https://github.com/Pylons/pyramid/issues/1223).
+
+        # clear old generated request.response, if any; it may
+        # have been mutated by the view, and its state is not
+        # sane (e.g. caching headers)
+        with hide_attrs(request, 'exception', 'exc_info', 'response'):
+            # we use .get instead of .__getitem__ below due to
+            # https://github.com/Pylons/pyramid/issues/700
+            request_iface = attrs.get('request_iface', IRequest)
+            response = _call_view(
+                registry,
+                request,
+                exc_info[0],
+                context_iface,
+                view_name,
+                view_types=None,
+                view_classifier=IExceptionViewClassifier,
+                secure=secure,
+                request_iface=request_iface.combined,
+                )
+            return response