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

[andrey.gtx@…]

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.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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.

[Aymeric Augustin]

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.