#26306 closed Bug (fixed)
Memory leak in cached template loader
Reported by: | Alex Hill | Owned by: | Alex Hill |
---|---|---|---|
Component: | Template system | Version: | 1.9 |
Severity: | Release blocker | Keywords: | |
Cc: | berker.peksag@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (9)
comment:1 by , 9 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
comment:2 by , 9 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 9 years ago
Cc: | added |
---|
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 usingsys.exc_info()
.
comment:4 by , 9 years ago
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.
comment:5 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 9 years ago
Needs tests: | unset |
---|
More substantial patch: https://github.com/django/django/pull/6259
comment:7 by , 9 years ago
Patch needs improvement: | unset |
---|
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