Opened 5 years ago

Closed 5 years ago

#17303 closed Bug (fixed)

Cached template loader can fail to find template in multi-threaded environment

Reported by: andrey.gtx@… Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Cached template loader resolves subloaders on demand and appends them to cached_loaders list immediately. This can lead to the usage of incomplete loader list while handling concurrent requests producing infrequent 'template not found'-exceptions.

Attachments (2)

patch.diff (722 bytes) - added by anonymous 5 years ago.
17303.diff (1012 bytes) - added by akaariai 5 years ago.

Download all attachments as: .zip

Change History (5)

Changed 5 years ago by anonymous

comment:1 Changed 5 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Two comments about the patch, based on just reading the diff:

  • A comment in there warning about concurrency issues would be warranted.
  • The patch allows multiple loading of the cached_loaders list. I think this is OK, as find_template() (django.template.loader) does something similar:
    global template_source_loaders
    if template_source_loaders is None:
        loaders = []
        for loader_name in settings.TEMPLATE_LOADERS:
            loader = find_template_loader(loader_name)
            if loader is not None:

So, marking as accepted. A testcase would be good, but it would also be a bit hard to write (testing race-conditions isn't too easy). I will try to review this fully tomorrow. I will see if I can find a clean way to write a testcase + of course try to apply & test the patch.

comment:2 Changed 5 years ago by akaariai

  • Triage Stage changed from Accepted to Ready for checkin

I added a comment in the patch, and made the patch apply to root directory. I think this is ready for checkin, although the committer might want to check the added comment. My English is what it is...

I tried to make the loaders property a cached_property. This would work fine, but tests use the _cached_loaders variable, and I do not have the time to fix the tests. This would be a good thing to fix, but I just don't have time to do that currently. Tests are effectively dependent on the implementation details of cached template loader, but they could almost as easily use the Loader.__init__ to pass the wanted loaders.

I also checked what it would take to write a test for the concurrency issue. This would require a dummy template loader. Add it as a second loader for the cached loader. Then (using a semaphore, for example) make two threads access the loaders() property with proper timing, so that the first thread has loaded just one template loader when the second thread access the loaders() property... In short: doable, but not worth the effort IMHO. The added comment should be guard enough against regressions.

All tests passed on sqlite3.

Changed 5 years ago by akaariai

comment:3 Changed 5 years ago by aaugustin

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

In [17295]:

Fixed #17303 -- Ensured the list of template loaders is fully loaded before it is cached. Thanks andrey DOT gtx AT gmail DOT com for the report and patch, and Anssi Kääriäinen for the review.

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