Opened 13 years ago

Closed 13 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: dev
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

Description

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 13 years ago.
17303.diff (1012 bytes ) - added by Anssi Kääriäinen 13 years ago.

Download all attachments as: .zip

Change History (5)

by anonymous, 13 years ago

Attachment: patch.diff added

comment:1 by Anssi Kääriäinen, 13 years ago

Triage Stage: UnreviewedAccepted

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:
                loaders.append(loader)
    

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 by Anssi Kääriäinen, 13 years ago

Triage Stage: AcceptedReady 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.

by Anssi Kääriäinen, 13 years ago

Attachment: 17303.diff added

comment:3 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: newclosed

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