Opened 12 years ago
Closed 12 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)
Change History (12)
by , 12 years ago
| Attachment: | #19510-thread_safe_template_cache.diff added |
|---|
comment:1 by , 12 years ago
| Cc: | added |
|---|
comment:2 by , 12 years ago
| Resolution: | → duplicate |
|---|---|
| Status: | new → closed |
comment:3 by , 12 years ago
| Needs tests: | set |
|---|---|
| Resolution: | duplicate |
| Status: | closed → 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 by , 12 years ago
Under normal use of Django (ie. outside tests), when does the cached template loader's cache get reset?
by , 12 years ago
| Attachment: | #19510-thread_safe_template_cache.2.diff added |
|---|
comment:5 by , 12 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.
comment:6 by , 12 years ago
| Component: | Uncategorized → Template system |
|---|---|
| Patch needs improvement: | set |
| Triage Stage: | Unreviewed → Accepted |
| Type: | Uncategorized → 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.
by , 12 years ago
| Attachment: | #19510-thread_safe_template_cache.3.diff added |
|---|
Simpler way to avoid race.
comment:7 by , 12 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
comment:8 by , 12 years ago
| Keywords: | sprint2013 added |
|---|
comment:9 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
#19511 seems to be a duplicate but it contains more info so I'll close this one.