Code

#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 19 months ago.
#19510-thread_safe_template_cache.2.diff (1.3 KB) - added by Kronuz 18 months ago.
#19510-thread_safe_template_cache.3.diff (705 bytes) - added by regebro 17 months ago.
Simpler way to avoid race.

Download all attachments as: .zip

Change History (12)

Changed 19 months ago by Kronuz

comment:1 Changed 19 months ago by Kronuz

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

comment:2 Changed 19 months 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 19 months 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 19 months ago by aaugustin

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

Changed 18 months ago by Kronuz

comment:5 Changed 18 months 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 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 18 months ago by Kronuz (previous) (diff)

comment:6 Changed 18 months 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 17 months ago by regebro

Simpler way to avoid race.

comment:7 Changed 17 months 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.

Version 0, edited 17 months ago by regebro (next)

comment:8 Changed 17 months ago by regebro

  • Keywords sprint2013 added

comment:9 Changed 16 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.