Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

request.exception is cleared too early #1223

Closed
mmerickel opened this Issue Jan 16, 2014 · 9 comments

Comments

Projects
None yet
4 participants
Owner

mmerickel commented Jan 16, 2014

currently it's cleared in the excview so finished callbacks cannot inspect the property

Owner

mcdonc commented Jan 22, 2014

We might be able to just remove the finally: clause in excview_tween which deletes exc_info and exception from the request attrs. I tried doing just this, with the following test app, which I hit repeatedly without seeing appreciable memory growth over time:

from wsgiref.simple_server import make_server
from pyramid.config import Configurator
from pyramid.response import Response

def hello_world(request):
    class Bar(object):
        pass
    raise ValueError(Bar())

def excview(request):
    print "responding ok"
    return Response('OK')

if __name__ == '__main__':
    config = Configurator()
    config.add_route('hello', '/')
    config.add_view(hello_world, route_name='hello')
    config.add_view(excview, context=ValueError)
    app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 8080, app)
    server.serve_forever()

That said, it's weird because with the finally: clause enabled (deleting the attributes) the process stays on my machine at 16MB, without the finally: clause enabled (not deleting the attributes), it grows to 17MB (but stays there). At the same time, I don't think there's any leak here. I just can't account for the extra MB.

Owner

mmerickel commented Jan 22, 2014

I was thinking the tween could just register a finished callback to clear it, but if we don't need to clear it at all that's nice too.

Owner

mcdonc commented Jan 22, 2014

Can you run the script above and see if you see the same 1MB descrepancy (via top) with and without the following patch?

--- a/pyramid/tweens.py
+++ b/pyramid/tweens.py
@@ -40,10 +40,11 @@ def excview_tween_factory(handler, registry):
             response = view_callable(exc, request)
         finally:
             # prevent leakage (wrt exc_info)
-            if 'exc_info' in attrs:
-                del attrs['exc_info']
-            if 'exception' in attrs:
-                del attrs['exception']
+            if 0:
+                if 'exc_info' in attrs:
+                    del attrs['exc_info']
+                if 'exception' in attrs:
+                    del attrs['exception']

         return response
Contributor

flibustenet commented Jan 22, 2014

I did the test with ab -n 10000, it grows a little but adding gc.collect in the end came back to 16M.

Owner

mcdonc commented Jan 22, 2014

You added gc.collect() somewhere in excview_tween?

Contributor

flibustenet commented Jan 23, 2014

Yes, i removed del attrs and added a finished_callback on each request to gc.collect. But it's only to test, it slow down a lot !!!

Contributor

jinty commented Jan 23, 2014

I'm +1 on not deleting the exception, but just removing the finally clause does cause a reference cycle. The comment is misleading, it is not a "leak" as the garbage collector does break the cycle and clean it. The python docs (see note at http://docs.python.org/2/library/sys.html#sys.exc_info) do say that these cycles are not "efficient", so maybe best to avoid it.

The cycle is exc_info -> traceback -> function locals -> attrib -> exc_info

Have a look at the output of this test program with and without the finally clause. On my test 6 objects were collected each request with the finally clause and 57 without.

from wsgiref.simple_server import make_server
from pyramid.config import Configurator
from pyramid.response import Response

import gc
gc.disable() # stop cyclic garbage collection

def hello_world(request):
    print 'Collecting %s %s %s objects' % (gc.collect(0), gc.collect(1), gc.collect(2))
    class Bar(object):
        pass
    raise ValueError(Bar())

def excview(request):
    print "responding ok"
    return Response('OK')

if __name__ == '__main__':
    config = Configurator()
    config.add_route('hello', '/')
    config.add_view(hello_world, route_name='hello')
    config.add_view(excview, context=ValueError)
    app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 8080, app)
    print 'serving'
    server.serve_forever()

I think this code can be written more simply by moving the exception handling logic to a separate function like the _handle_error in pyramid_exclog (https://github.com/Pylons/pyramid_exclog/blob/master/pyramid_exclog/__init__.py#L108). I'll try work up a pull request like that when I next get a moment (may be a while :( ).

Owner

mmerickel commented Jan 23, 2014

psuedocode:

try:
    response = handler(request)
except:
    request.exc_info = sys.exc_info()
    def callback(request):
        if hasattr(request, 'exc_info'):
            del request.exc_info
    request.add_finished_callback(callback)
    raise
Owner

mcdonc commented Feb 8, 2014

I think we can chalk the extra MB up to the extra work that needs to be done by the garbage collector. I think it's probably best to rely on the garbage collector to notice the cycle and break it rather than add a finished callback (if only because add_finished_callback is really a user API and having the framework register one is a little weird).

@mcdonc mcdonc closed this in 579a5f4 Feb 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment