Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10758 closed (fixed)

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

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

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 5 years ago.
fixes both issues.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to HTTP handling
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by guettli

  • Cc hv@… added

Changed 5 years ago by kevinh

fixes both issues.

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by kevinh

  • Has patch set
  • Version changed from 1.0 to SVN

comment:6 Changed 5 years ago by lrekucki

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 Changed 5 years ago by 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?).

comment:8 Changed 5 years ago by 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.

comment:9 Changed 5 years ago by lukeplant

  • Owner changed from nobody to lukeplant
  • Status changed from new to assigned

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 Changed 5 years ago by Alex

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 Changed 5 years ago by Alex

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

comment:12 Changed 5 years ago by Alex

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

comment:13 Changed 5 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 Changed 5 years ago by guettli

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