{% ifchanged %} not thread-safe

[Antti Kaihola]

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

[Antti Kaihola]

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

[Alex Gaynor]

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.

[Antti Kaihola]

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.

[Antti Kaihola]

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

[Carl Meyer]

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

[Antti Kaihola]

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?

[Carl Meyer]

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.

[Diederik van der Boor]

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. :-/

[Carl Meyer]

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.

[Diederik van der Boor]

There is a pull request at ​https://github.com/django/django/pull/764, written in cooperation with @bpeschier

[Aymeric Augustin]

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

[Florian Apolloner]

See the comment on the pull request.

[Florian Apolloner <florian@…>]

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.