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)

15849-ifchanged-not-thread-safe.1.diff (1.4 KB ) - added by Antti Kaihola 14 years ago.
Fixes #15849 -- use context.render_context instead of self for storing state in IfChangedNode
15849-ifchanged-not-thread-safe.2.diff (1.6 KB ) - added by Antti Kaihola 14 years ago.
Fixes #15849 -- use context.render_context instead of self for storing state in IfChangedNode. Also now use self instead of str(id(self)) as the key when storing the node in contextforloop`. Patch is against r16046.

Download all attachments as: .zip

Change History (16)

by Antti Kaihola, 14 years ago

Fixes #15849 -- use context.render_context instead of self for storing state in IfChangedNode

comment:1 by Antti Kaihola, 14 years ago

Has patch: set

comment:2 by Alex Gaynor, 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 Antti Kaihola, 14 years ago

Fixes #15849 -- use context.render_context instead of self for storing state in IfChangedNode. Also now use self instead of str(id(self)) as the key when storing the node in contextforloop`. Patch is against r16046.

in reply to:  2 comment:3 by Antti Kaihola, 14 years ago

Replying to Alex:

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.

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)

comment:4 by Carl Meyer, 14 years ago

Easy pickings: unset
Needs tests: set
Triage Stage: UnreviewedAccepted

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 ;-)

in reply to:  4 ; comment:5 by Antti Kaihola, 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?

in reply to:  5 comment:6 by Carl Meyer, 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 Jannis Leidel, 13 years ago

Patch needs improvement: set
UI/UX: unset

comment:8 by Diederik van der Boor, 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. :-/

in reply to:  8 comment:9 by Carl Meyer, 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 Bas Peschier, 12 years ago

Keywords: sprint2013 added
Owner: changed from nobody to Bas Peschier
Status: newassigned

comment:11 by Diederik van der Boor, 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 Aymeric Augustin, 12 years ago

Triage Stage: AcceptedReady for checkin

Discussed at the Utrecht sprint, this is probably good to go (as fas as a 5-minute code review goes).

comment:13 by Florian Apolloner, 12 years ago

Patch needs improvement: set

See the comment on the pull request.

comment:14 by Florian Apolloner <florian@…>, 12 years ago

Resolution: fixed
Status: assignedclosed

In 8503120c1024ad7ec2151196a743d6daec21334b:

Fixed #15849 -- Made IfChanged node thread safe.

Previously, the ifchanged node stored state on self._last_seen,
thereby giving undesired results when the node is reused by another
thread at the same time (e.g. globally caching a Template object).

Thanks to akaihola for the report and Diederik van der Boor and
Bas Peschier for the patch.

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