Code

Opened 14 months ago

Closed 8 months ago

#19949 closed Cleanup/optimization (fixed)

Cached template loader doesn't cache TemplateDoesNotExist

Reported by: Kronuz Owned by: susan
Component: Template system Version: 1.5
Severity: Normal Keywords: templates cache
Cc: Kronuz 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)

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 Kronuz 14 months ago.

Download all attachments as: .zip

Change History (14)

Changed 14 months ago by Kronuz

comment:1 Changed 14 months ago by Kronuz

  • Cc Kronuz added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 14 months ago by Kronuz

  • Component changed from Uncategorized to Template system
  • Has patch set
  • Keywords templates cache added
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 14 months ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 9 months ago by timo

  • Needs tests set

It would be good to add a test for this.

comment:5 Changed 8 months ago by susan

https://github.com/django/django/pull/1501 I'm still debugging, I'm getting an error on this line L135:

template_tuple = cache_loader.load_template('missing.html', "django/tests/missing")

File "/../django/template/loaders/cached.py", line 27, in loaders

for loader in self._loaders:

TypeError: 'type' object is not iterable

I'll appreciate any feedback.

Version 0, edited 8 months ago by susan (next)

comment:6 Changed 8 months ago by timo

  • Needs tests unset
  • Patch needs improvement set

comment:7 Changed 8 months ago by susan

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

comment:8 Changed 8 months ago by timo

  • Patch needs improvement unset
  • Resolution set to fixed
  • Status changed from assigned to closed

comment:9 Changed 8 months ago by ramiro

  • Description modified (diff)

comment:10 Changed 8 months ago by ramiro

  • Resolution fixed deleted
  • Status changed from closed to new

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 8 months ago by ramiro (previous) (diff)

comment:11 Changed 8 months ago by susan

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 Changed 8 months ago by Ramiro Morales <cramm0@…>

In b785a80d1934ef05165f222779c3f47c32889825:

Added further fixes, tests for #19949/f33db5a09a.

Thanks Susan Tan. Refs #19949.

comment:13 Changed 8 months ago by ramiro

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.