Opened 15 years ago

Closed 14 years ago

Last modified 13 years ago

#10758 closed (fixed)

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

Reported by: piotr.findeisen@… Owned by: Luke Plant
Component: HTTP handling Version: dev
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Attachments (1)

patch.diff (1.1 KB ) - added by kevinh 14 years ago.
fixes both issues.

Download all attachments as: .zip

Change History (15)

comment:1 by Thejaswi Puthraya, 15 years ago

Component: UncategorizedHTTP handling

comment:2 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Thomas Güttler, 14 years ago

Cc: hv@… added

by kevinh, 14 years ago

Attachment: patch.diff added

fixes both issues.

comment:4 by kevinh, 14 years ago

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.

comment:5 by kevinh, 14 years ago

Has patch: set
Version: 1.0SVN

comment:6 by Łukasz Rekucki, 14 years ago

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.

comment:7 by Skaffen, 14 years ago

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?).

comment:8 by kevinh, 14 years ago

"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.

comment:9 by Luke Plant, 14 years ago

Owner: changed from nobody to Luke Plant
Status: newassigned

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.

comment:10 by Alex Gaynor, 14 years ago

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.

comment:11 by Alex Gaynor, 14 years ago

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

comment:12 by Alex Gaynor, 14 years ago

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

comment:13 by Luke Plant, 14 years ago

Resolution: fixed
Status: assignedclosed

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

Thanks piotr.findeisen for report, kevinh for patch.

comment:14 by Thomas Güttler, 13 years ago

Cc: hv@… removed
Note: See TracTickets for help on using tickets.
Back to Top