Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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 Changed 4 years ago by Alex Hill

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:2 Changed 4 years ago by Tim Graham

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Berker Peksag

Cc: berker.peksag@… 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:

comment:4 Changed 4 years ago by Alex Hill

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

Owner: changed from nobody to Alex Hill
Status: newassigned

comment:6 Changed 4 years ago by Alex Hill

Needs tests: unset

comment:7 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In ecb59cc6:

Fixed #26306 -- Fixed memory leak in cached template loader.

comment:9 Changed 4 years ago by Tim Graham <timograham@…>

In f49cfb76:

[1.9.x] Fixed #26306 -- Fixed memory leak in cached template loader.

Backport of ecb59cc6579402b68ddfd4499bf30edacf5963be from master

Note: See TracTickets for help on using tickets.
Back to Top