Opened 11 years ago

Closed 11 years ago

#19949 closed Cleanup/optimization (fixed)

Cached template loader doesn't cache TemplateDoesNotExist

Reported by: German M. Bravo Owned by: Susan Tan
Component: Template system Version: 1.5
Severity: Normal Keywords: templates cache
Cc: German M. Bravo Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

In cases where multiple fallback templates are requested to the template loader (i.e. django.template.loader.select_template()) to find the first one available, the cached template loader does not cache non-existent templates, making cached loader "slow" again if every time it has to try looking for a template that's been already found to be missing.

from django.template.loader import render_to_string
render_to_string(['missing_template_1.html', 'missing_template_2.html', ..., 'missing_template_N.html', 'existent_template.html'])

When using django.template.loaders.cached.Loader, and given that snippet above, and the fact that missing_template_1.html ... missing_template_N.html do not exist, the filesystem loader or other loaders get hit every time (trying and trying to find already known as missing templates).

I'm attaching a patch to fix this.

Attachments (1)

#19949-cache_non_existent_templates.diff (2.3 KB ) - added by German M. Bravo 11 years ago.

Download all attachments as: .zip

Change History (14)

by German M. Bravo, 11 years ago

comment:1 by German M. Bravo, 11 years ago

Cc: German M. Bravo added

comment:2 by German M. Bravo, 11 years ago

Component: UncategorizedTemplate system
Has patch: set
Keywords: templates cache added
Type: UncategorizedCleanup/optimization

comment:3 by Jacob, 11 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Tim Graham, 11 years ago

Needs tests: set

It would be good to add a test for this.

comment:5 by Susan Tan, 11 years ago

Updated PR: https://github.com/django/django/pull/1501/ Unit test and full test suite pass.

Last edited 11 years ago by Susan Tan (previous) (diff)

comment:6 by Tim Graham, 11 years ago

Needs tests: unset
Patch needs improvement: set

comment:7 by Susan Tan, 11 years ago

Owner: changed from nobody to Susan Tan
Status: newassigned

comment:8 by Tim Graham, 11 years ago

Patch needs improvement: unset
Resolution: fixed
Status: assignedclosed

comment:9 by Ramiro Morales, 11 years ago

Description: modified (diff)

comment:10 by Ramiro Morales, 11 years ago

Resolution: fixed
Status: closednew

I believe we need to work on this ticket a bit more.

The tests added, although correct because they verify something we didn't test before, don't actually test the fix introduced and so didn't fail beforehand.

In particular these two lines:

https://github.com/django/django/blob/master/django/template/loaders/cached.py#L74-L75

gives us a hint about the fact that this needs further investigation.

I've opened a PR with tests changes. We can discuss things there if you wish: https://github.com/django/django/pull/1511

It's not clear to me yet what the right fix would look like. Maybe we can reuse some code from the patch proposed by the OP (refactored to accompany the code evolution)?

Reopening.

Last edited 11 years ago by Ramiro Morales (previous) (diff)

comment:11 by Susan Tan, 11 years ago

Thanks for catching the missing test. I forgot we had to test the cache after the loaders tries to load the missing template. I made a few comments on your PR.

comment:12 by Ramiro Morales <cramm0@…>, 11 years ago

In b785a80d1934ef05165f222779c3f47c32889825:

Added further fixes, tests for #19949/f33db5a09a.

Thanks Susan Tan. Refs #19949.

comment:13 by Ramiro Morales, 11 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top