[Django] #26306: Memory leak in cached template loader
Django
---------------------------------+--------------------
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
There's a pretty serious memory leak in the cached template loader.
Here's a bare-bones project demonstrating the issue:
https://github.com/AlexHill/django_leak_test
To see it happen, run that project under 1.9, hit /safe/ a couple thousand
times, then try the same with /leaky/.
Here's where it happens:
https://github.com/django/django/blob/6679cdd92c71a77f1809c180174de026a6c17918/django/template/loaders/cached.py#L34
I think what's happening is this:
When the exception is raised, it contains a stack trace, which contains
references to a whole lot of stuff up the stack including the
TemplateResponse, the request, etc. That's all cached with the exception.
When it's fetched from the cache and re-raised, Python's exception
handling mechanics result in the exception containing references to the
current stack AND the previous one.
This happens every time it's fetched from cache, so essentially you end up
caching the response and context for every request that produced a
TemplateDoesNotExist error while resolving a template name. In my case,
that's basically every request :)
I think caching and re-raising the actual exception is probably a mistake
- instead, we should cache some sentinel object and raise a new exception
each time.
--
Ticket URL: <https://code.djangoproject.com/ticket/26306>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: => 1
* has_patch: 0 => 1
* needs_tests: => 1
* needs_docs: => 0
Comment:
And here's a patch against 1.9.x that plugs the leak:
https://github.com/django/django/compare/stable/1.9.x...AlexHill:template_mem_leak
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:1>
Django
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:2>
Django
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by berkerpeksag):
* cc: berker.peksag@… (added)
Comment:
Some comments about the patch:
* `self.missing_sentinel` is unused.
* `TemplateMissingMarker` could live in module level.
* There still will be some memory unnecessary memory usage because of
`TemplateMissingMarker`. Perhaps we could define `__slots__` to reduce
memory usage.
Random thoughts:
* I wonder we could use `traceback.clear_frames()`:
https://docs.python.org/3.5/library/traceback.html#traceback.clear_frames
Its implementation is simple:
https://github.com/python/cpython/blob/master/Lib/traceback.py#L212
* Instead of storing whole `TemplateDoesNotExist` object, we could only
store exception class and values (not traceback object) in a tuple by
using `sys.exc_info()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:3>
Django
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by AlexHill):
Thanks for your comments!
I wrote a detailed comment here last night, lost it due to not being
logged in in this tab, then ragequit in frustration.
The gist is: even with the leak plugged, 1.9 can cause significantly
higher memory usage than the same project using 1.8, because template
debug information is always cached. We can fix it by not caching debug
information when template debugging is off.
In 1.8, the thing that is cached when a template isn't found is the
TemplateDoesNotExist class itself. When the debug view wants to know which
paths were searched while looking for the template, it consults the
rendering engine again. In 1.9, that debug information is stored on an
exception instance, and those exceptions are cached.
I use Mezzanine, which tries a lot of different templates for each content
page, in order to allow you to override templates in various ways.
Significantly, it tries a template matching the exact slug of each page,
so that page templates can be individually overridden. In my case, I have
about 100,000 such pages, of which only a handful are overridden, so the
template cache is mostly full of "template missing" errors.
With app directory template discovery, 100,000 pages and about 50 items in
INSTALLED_APPS, I end up with ~5m template paths being cached, along with
their wrapping `Origin` objects, tuples, etc. Within a day or so this eats
most of the memory on my server.
I think it makes sense to have this information in the exception, but it's
debugging information and doesn't need to be stored when template
debugging is available. I propose restoring the old caching behaviour when
template debugging is off so that the overhead per missing template is
simply one entry in a dict.
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:4>
Django
Reporter: AlexHill | Owner: AlexHill
Type: Bug | Status: assigned
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by AlexHill):
* owner: nobody => AlexHill
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:5>
Django
Reporter: AlexHill | Owner: AlexHill
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_tests: 1 => 0
Comment:
More substantial patch: https://github.com/django/django/pull/6259
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:6>
Django
Reporter: AlexHill | Owner: AlexHill
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:7>
Django
Reporter: AlexHill | Owner: AlexHill
Component: Template system | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ecb59cc6579402b68ddfd4499bf30edacf5963be" ecb59cc6]:
{{{
#!CommitTicketReference repository=""
revision="ecb59cc6579402b68ddfd4499bf30edacf5963be"
Fixed #26306 -- Fixed memory leak in cached template loader.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:8>
Django
Reporter: AlexHill | Owner: AlexHill
Type: Bug | Status: closed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f49cfb76c7b24a4fa5f8ac36dfa0a82ab66336c5" f49cfb76]:
{{{
#!CommitTicketReference repository=""
revision="f49cfb76c7b24a4fa5f8ac36dfa0a82ab66336c5"
[1.9.x] Fixed #26306 -- Fixed memory leak in cached template loader.
Backport of ecb59cc6579402b68ddfd4499bf30edacf5963be from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26306#comment:9>