Opened 2 years ago

Closed 2 years ago

#19510 closed Bug (fixed)

Thread safe template cache

Reported by: Kronuz Owned by: nobody
Component: Template system Version: 1.4
Severity: Normal Keywords: sprint2013
Cc: Kronuz Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I've come across a problem that pops up some times, when a thread clears the cache by resetting the cached templates loader while other is loading it.
I made a patch for this.

Attachments (3)

#19510-thread_safe_template_cache.diff (1.4 KB) - added by Kronuz 2 years ago.
#19510-thread_safe_template_cache.2.diff (1.3 KB) - added by Kronuz 2 years ago.
#19510-thread_safe_template_cache.3.diff (705 bytes) - added by regebro 2 years ago.
Simpler way to avoid race.

Download all attachments as: .zip

Change History (12)

Changed 2 years ago by Kronuz

comment:1 Changed 2 years ago by Kronuz

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

comment:2 Changed 2 years ago by apollo13

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

#19511 seems to be a duplicate but it contains more info so I'll close this one.

comment:3 Changed 2 years ago by apollo13

  • Needs tests set
  • Resolution duplicate deleted
  • Status changed from closed to new

Doesn't seem to be a duplicate, only the patch seems to be the same which wouldn't make sense. Can you get into more details about the error?

comment:4 Changed 2 years ago by aaugustin

Under normal use of Django (ie. outside tests), when does the cached template loader's cache get reset?

Changed 2 years ago by Kronuz

comment:5 Changed 2 years ago by Kronuz

I added a cleaner patch (the old one had bits of other patch in it). I'm not, however, aware of anywhere where the cache might get reset, I looked around and couldn't find any place in my own code either, but the KeyError exception was happening, and after these days the patch completely fixed it.

I'm adding the correct patch to the other issue (#19511) as well.

Version 0, edited 2 years ago by Kronuz (next)

comment:6 Changed 2 years ago by akaariai

  • Component changed from Uncategorized to Template system
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

To me it seems the patch is changing the semantics of load_template(). The origin is cached and returned always while that isn't the case before. There is no explanation if or why the change is needed.

However, there is a clear race in there - we check if the key exists in the dict, then assume it still exists a couple of lines later. So, clear() in between will cause KeyError.

Marking as accepted based on the race. Testing this seems a little complex, as we will need to time the reset() very accurately to hit this problem - so I don't think a test is absolutely necessary.

Changed 2 years ago by regebro

Simpler way to avoid race.

comment:7 Changed 2 years ago by regebro

  • Has patch set
  • Needs tests unset

As mentioned it's near impossible to test race conditions, so I don't have a test. But I have attached a simpler fix for the race condition, by avoiding the separation of test and return. It would be good if Kronuz could test this.

Try-Except should be slower though, but unless you are rendering thousands of small templates in one view that should not make a measurable difference, but only performance tests can determine that.

I made a branch as well: https://github.com/regebro/django/tree/issue-19510

Last edited 2 years ago by regebro (previous) (diff)

comment:8 Changed 2 years ago by regebro

  • Keywords sprint2013 added

comment:9 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

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

In 68905695b897e62b0c18d9edd87171a0eae4e67e:

Fixed #19510 -- Race condition in template loading.

Thanks Kronuz and regebro.

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