Opened 11 years ago

Closed 11 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 11 years ago.
#19510-thread_safe_template_cache.2.diff (1.3 KB ) - added by German M. Bravo 11 years ago.
#19510-thread_safe_template_cache.3.diff (705 bytes ) - added by regebro 11 years ago.
Simpler way to avoid race.

Download all attachments as: .zip

Change History (12)

by German M. Bravo, 11 years ago

comment:1 by German M. Bravo, 11 years ago

Cc: German M. Bravo added

comment:2 by Florian Apolloner, 11 years ago

Resolution: duplicate
Status: newclosed

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

comment:3 by Florian Apolloner, 11 years ago

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

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

by German M. Bravo, 11 years ago

comment:5 by German M. Bravo, 11 years ago

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 using it all these days the patch completely fixed it.

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

Last edited 11 years ago by German M. Bravo (previous) (diff)

comment:6 by Anssi Kääriäinen, 11 years ago

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.

by regebro, 11 years ago

Simpler way to avoid race.

comment:7 by regebro, 11 years ago

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 11 years ago by regebro (previous) (diff)

comment:8 by regebro, 11 years ago

Keywords: sprint2013 added

comment:9 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

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