Code

Opened 3 years ago

Closed 14 months ago

#15849 closed Bug (fixed)

{% ifchanged %} not thread-safe

Reported by: akaihola Owned by: bpeschier
Component: Template system Version: master
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 akaihola 3 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 akaihola 3 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)

Changed 3 years ago by akaihola

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

comment:1 Changed 3 years ago by akaihola

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 follow-up: Changed 3 years ago by 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.

Changed 3 years ago by akaihola

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.

comment:3 in reply to: ↑ 2 Changed 3 years ago by akaihola

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 follow-up: Changed 3 years ago by carljm

  • Easy pickings unset
  • Needs tests set
  • Triage Stage changed from Unreviewed to 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 ;-)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by akaihola

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 in reply to: ↑ 5 Changed 3 years ago by carljm

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 Changed 3 years ago by jezdez

  • Patch needs improvement set
  • UI/UX unset

comment:8 follow-up: Changed 2 years ago by vdboor

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 in reply to: ↑ 8 Changed 2 years ago by carljm

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 Changed 14 months ago by bpeschier

  • Keywords sprint2013 added
  • Owner changed from nobody to bpeschier
  • Status changed from new to assigned

comment:11 Changed 14 months ago by vdboor

  • 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 Changed 14 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:13 Changed 14 months ago by apollo13

  • Patch needs improvement set

See the comment on the pull request.

comment:14 Changed 14 months ago by Florian Apolloner <florian@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

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.