Opened 4 years ago

Closed 4 years ago

#19510 closed Bug (fixed)

Thread safe template cache

Reported by: German M. Bravo Owned by: nobody
Component: Template system Version: 1.4
Severity: Normal Keywords: sprint2013
Cc: German M. Bravo 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 German M. Bravo 4 years ago.
#19510-thread_safe_template_cache.2.diff (1.3 KB) - added by German M. Bravo 4 years ago.
#19510-thread_safe_template_cache.3.diff (705 bytes) - added by regebro 4 years ago.
Simpler way to avoid race.

Download all attachments as: .zip

Change History (12)

Changed 4 years ago by German M. Bravo

comment:1 Changed 4 years ago by German M. Bravo

Cc: German M. Bravo added

comment:2 Changed 4 years ago by Florian Apolloner

Resolution: duplicate
Status: newclosed

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

comment:3 Changed 4 years ago by Florian Apolloner

Needs tests: set
Resolution: duplicate
Status: closednew

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 4 years ago by Aymeric Augustin

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

Changed 4 years ago by German M. Bravo

comment:5 Changed 4 years ago by German M. Bravo

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 4 years ago by German M. Bravo (next)

comment:6 Changed 4 years ago by Anssi Kääriäinen

Component: UncategorizedTemplate system
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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 4 years ago by regebro

Simpler way to avoid race.

comment:7 Changed 4 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 4 years ago by regebro (previous) (diff)

comment:8 Changed 4 years ago by regebro

Keywords: sprint2013 added

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

Resolution: fixed
Status: newclosed

In 68905695b897e62b0c18d9edd87171a0eae4e67e:

Fixed #19510 -- Race condition in template loading.

Thanks Kronuz and regebro.

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