Opened 14 years ago
Closed 12 years ago
#15849 closed Bug (fixed)
{% ifchanged %} not thread-safe
Reported by: | Antti Kaihola | Owned by: | Bas Peschier |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | sprint2013 |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
We're doing the wrong thing and running a Django site with mod_python on Apache's worker MPM. It turns out that {% ifchanged %} fails to group items correctly when there are concurrent requests.
In the implementation of {% ifchanged %} in Django 1.0 through current post-1.3 trunk [16044], an IfChangedNode
instance stores state in self._last_seen
. It should use context.render_context
instead (e.g. CycleNode
does this).
Attachments (2)
Change History (16)
by , 14 years ago
Attachment: | 15849-ifchanged-not-thread-safe.1.diff added |
---|
comment:1 by , 14 years ago
Has patch: | set |
---|
follow-up: 3 comment:2 by , 14 years ago
There is 0 need for this self._id
nonsense (in either the current or patched version), just replace with render_context[self]
, and enjoy the fruits of speed and not being dumb.
by , 14 years ago
Attachment: | 15849-ifchanged-not-thread-safe.2.diff added |
---|
comment:3 by , 14 years ago
Replying to Alex:
There is 0 need for this
self._id
nonsense (in either the current or patched version), just replace withrender_context[self]
, and enjoy the fruits of speed and not being dumb.
Fixed that. I wondered about str(id(self))
myself but decided not to try to be smarter than the author and committer (nedbatchelder and adrian, respectively) of the original code from [7752].
(BTW, mark-up preview for attachment descriptions would rock)
follow-up: 5 comment:4 by , 14 years ago
Easy pickings: | unset |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
I could see an argument that automated tests for template tag thread-safety or more difficult than they are worth, but I think we should at least have the argument ;-)
follow-up: 6 comment:5 by , 14 years ago
Replying to carljm:
I could see an argument that automated tests for template tag thread-safety or more difficult than they are worth, but I think we should at least have the argument ;-)
I found an existing thread-safety test case in the Django test suite. It's for bug #4948 ("Saving FileField files is not thread-safe"). Martin von Löwis also described in a comment how his example script demonstrated the bug.
Do you think a similar approach could be used for testing template tags?
comment:6 by , 14 years ago
Replying to akaihola:
I found an existing thread-safety test case in the Django test suite. It's for bug #4948 ("Saving FileField files is not thread-safe"). Martin von Löwis also described in a comment how his example script demonstrated the bug.
Do you think a similar approach could be used for testing template tags?
In general, yes, but (not having dug into this in detail) I'm not sure threading is even needed to check this particular case. Seems like the test could just create an IfChangedNode and a couple contexts and make the appropriate render() calls in the right order to trigger the bug, with a comment indicating that this simulates an order of calls that could occur under threaded conditions.
comment:7 by , 13 years ago
Patch needs improvement: | set |
---|---|
UI/UX: | unset |
follow-up: 9 comment:8 by , 13 years ago
Agreed with carljm. What is needed to get this bug fixed, only tests?
I find it a bit silly to see such threading bug open for 10+ months already. :-/
comment:9 by , 13 years ago
Replying to vdboor:
Agreed with carljm. What is needed to get this bug fixed, only tests?
The patch is currently marked both "needs tests" and "patch needs improvement", though jezdez didn't clarify what he thinks needs improving about the latest patch.
I find it a bit silly to see such threading bug open for 10+ months already. :-/
This is a tired song. It's easy to narrow your view to a single bug and make such declarations. There are many bugs in Trac, some of them open far longer than this one. The ones that get fixed are the ones that someone cares about enough to push forward. Apparently this threading bug doesn't actually bother anyone enough for that. If it bothers you, feel free to help by reviewing the patch, adding tests, etc.
comment:10 by , 12 years ago
Keywords: | sprint2013 added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:11 by , 12 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
There is a pull request at https://github.com/django/django/pull/764, written in cooperation with @bpeschier
comment:12 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Discussed at the Utrecht sprint, this is probably good to go (as fas as a 5-minute code review goes).
comment:14 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fixes #15849 -- use context.render_context instead of self for storing state in IfChangedNode