sys.exc_info() should not be stored on a local variable

[piotr.findeisen@…]

In get_response() function in file ​http://code.djangoproject.com/browser/django/trunk/django/core/handlers/base.py#L132 there is a code like this:

    ...
except:
    exc_info = sys.exc_info()
    receivers = signals.got_request_exception.send(sender=self.__class__, request=request)
    return self.handle_uncaught_exception(request, resolver, exc_info)

First: according to python docs ​http://docs.python.org/library/sys.html#sys.exc_info you should

• save only sys.exc_info()[:2] or
• wrap it in try-finally to ensure exc_info variable is always deleted

Second: why do you catch all exceptions instead of Exception? Is there a good reason to catch e.g. KeyboardInterrupt?

[kevinh]

fixes both issues.

[kevinh]

The memory leak is an issue I've seen in production, as recent as today... though I'm not sure I want to take the time to write tests for it as it will probably only present itself under load in python versions later than 2.2, and is already sort of hard to test in the first place.

On the subject of catching everything. The ticket submitter, was correct asking for it to just catch Exception. Just like how you don't generally derive your exceptions from BaseException, you generally don't want to be catching the everything that belongs to BaseException. I don't really care on that though, as it's kind of pedantic. I don't think this change would really have any great effect, unless you decided to do the opposite of the documentation and derive all of your errors from BaseException... you might have some issues.

tldr;
memory leak is fixed, but will be hard to test.
exception handling is fixed, but would be pedantic to test.

[Łukasz Rekucki]

The real problem with exception handling is that older versions of Python allowed any object to be raised as an exception. Python 2.5 still allows raising strings, see: ​http://docs.python.org/release/2.5.4/whatsnew/pep-352.html. While no one sane, would write new code that does that, I can imagine working with some legacy libraries.

As for the memory-leak, I really would like to see some tests, that cause it. AFAIK, the cycle will be garbage collected by Python sooner or later.

[Skaffen]

Off the top of my head - if there's a delay in the gc collecting the cycle and you get a few tracebacks it could spike the memory usage of the process and if it's python 2.4 it could result in that memory not being freed resulting in it looking like a memory leak?

Regardless of whether it's demonstrable as a cause of a memory leak, the python documentation surrounding the sys.exc_info/local variable reference cycle says "Beginning with Python 2.2, such cycles are automatically reclaimed when garbage collection is enabled and they become unreachable, but it remains more efficient to avoid creating cycles." - i.e. to me it suggests avoiding such cycles where possible, and as it seems a simple change in the code to avoid that cycle in this case then even if there's no test case proving a memory leak it would seem worthy to improve the code to make it more efficient. Perhaps a regression test (if one doesn't exist for that section of code) would be sufficient for that change?

This is, of course, separate from the question of whether the exception handling should just catch exceptions derived from BaseException - I don't think that should be in this ticket as although it's the same area of code it's a different issue (and I'm not sure a strong case has been made for why just things derived from BaseException should be caught - is there actually a problem?).

[kevinh]

"Off the top of my head - if there's a delay in the gc collecting the cycle and you get a few tracebacks it could spike the memory usage of the process and if it's python 2.4 it could result in that memory not being freed resulting in it looking like a memory leak?"

Yes we are seeing this in production, though with python 2.5. It only occurs under heavy load (we get around 10 million page views/month), and only if we push pages out that throw 500 errors. Though it is fairly consistent.

"As for the memory-leak, I really would like to see some tests, that cause it. AFAIK, the cycle will be garbage collected by Python sooner or later."

I would love to see some tests too. Though this is beyond me.

[Luke Plant]

Tests for this are probably going to be very hard or very fragile, so I'll commit without tests, given that you say this is causing real world problems. I'll it as "except:" for reasons of possible legacy libraries, as already noted.

[Alex Gaynor]

This patch isn't technically correct IMO, if an exception is raised in that signal, at any poinit, exc_info() will return that exception's info I think.

[Alex Gaynor]

Maybe not actually... , I'm not totally sure.

[Alex Gaynor]

err, looks like not. FWIW there's no potential for a memory leak here, the GC will eventually reclaim this memory.

[Luke Plant]

(In [13318]) Fixed #10758 - sys.exc_info() should not be stored on a local variable

Thanks piotr.findeisen for report, kevinh for patch.