Re: [Django] #7060: Tutorial skips over a race condition
Django
-------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
Changes (by garrison):
* status: closed => new
* severity: => Normal
* resolution: invalid =>
* easy: => 0
* ui_ux: => 0
* type: => Uncategorized
Comment:
I am re-opening this, as I think the decision here ought to be revisited.
It reflects poorly on Django for the tutorial to teach poor coding
practices, particularly when there is a race condition in a vote-tallying
app. More and more, such race conditions are being treated as true
security vulnerabilities.
Following
[http://www.reddit.com/r/programming/comments/33zg4u/race_conditions_on_facebook_digitalocean_and/cqpxnpp
discussion on reddit], I suggest putting a note in the tutorial mentioning
that the code indeed has a race condition, and that more advanced users
should consider Django's F-expressions instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
* easy: 0 => 1
Comment:
I think changing the `vote()` view to use F expressions is a good idea. We
could link to `docs/ref/models/expressions/#avoiding-race-conditions-
using-f` for a more complete explanation.
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:8>
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by bmispelon):
Personally I think adding `F` expressions at this point in the tutorial is
premature and too complicated.
The `vote` view has no user authenticated and no rate-limiting so adding a
protection against race conditions seems pointless to me.
I'm not sure the argument about better coding practices applies here
either because if that's what we were going for, we'd be using a Form
instead of the current ad-hoc handling.
However I would be OK with a paragraph mentionning the limitations of the
code and linking to things like forms, F objects, ...
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by garrison):
The `vote` view has no user authenticated and no rate-limiting so adding a
protection against race conditions seems pointless to me.
I think it is reasonable to expect people going through the tutorial to
recognize that the voting app has no rate-limiting or user authentication.
On the other hand, I do //not// think it is reasonable to expect a
developer new to Django to recognize that there is a race condition in the
vote tallying code. Since it involves the ORM layer (which is largely a
black box at this point in the tutorial), new developers might even get
the impression that the lines `selected_choice.votes += 1` and
`selected_choice.save()` lead to an atomic operation being performed on
the database, and are therefore the preferred way of doing such
operations. With a more complex ORM, this might be a reasonable
assumption, but it is not the way Django works.
In any event, it seems that we each agree that a note should be added to
the tutorial (at a minimum), so there is probably no reason to belabor
these points further.
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:10>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* owner: nobody => raphaelm
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: raphaelm
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by raphaelm):
https://github.com/django/django/pull/4756 is a proposal for a note in the
tutorial. Feel free to suggest any changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: raphaelm
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:13>
Django
--------------------------------------+------------------------------------
Reporter: donald.ball@… | Owner: raphaelm
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3e9b5bfd9c3e16c968278f7d050f52739690eb29" 3e9b5bfd]:
{{{
#!CommitTicketReference repository=""
revision="3e9b5bfd9c3e16c968278f7d050f52739690eb29"
Fixed #7060 -- Added a note about race conditions to the tutorial
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/7060#comment:14>