[Django] #22967: Model._do_update not always consistent with the precise meaning of returned value
Django
value
----------------------------------------------+--------------------
Reporter: knaperek | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
''This is a bit obscure corner-case bug which might seem unimportant, but
the solution is easy and actually only makes the code simpler (removes one
nested if) and more consistent.''
When {{{select_on_save}}} is set, {{{Model._do_update()}}} ignores the
result of {{{Model._update()}}} if it learnt that the object exists '''in
a previous DB call'''. That last thing is crucial, as the object could
have already been deleted by concurrent worker just in-between these two
ORM calls ({{{exists}}} and {{{_update}}}). In such case, the
{{{_update()}}} call will fail, resulting in nothing being actually
updated. However, {{{_do_update()}}} method will currently return
'''True''' regardless of {{{_update()}}} result, which is inconsistent
with the common (and expected) behavior, and also in contrast with the
method's docs (saying: ''If the model was updated (in the sense that an
update query was done and a matching row was found from the DB) the method
will return True.'')
As a consequence, object will not be saved into the database, because
{{{_save_table()}}} won't call {{{_do_insert()}}}, being misled by
{{{_do_update()}}}. Now you may oppose that this does not really matter
that much, since it has been caused by concurrent modifications of the
same data, which is considered non-deterministic anyway (so noone has the
right to complain that the object is not there at the end). However,
here's one example that vouches for this bug to be fixed: if you happen to
have a trigger in your DB counting the number of times an object was
changed (updated/inserted), you'd be missing one (which is no longer
justifiable by concurrent save() and delete() operations).
As it sometimes happens, it is IMHO impossible to create a unit test for
this bug, as it is too low level thing, exploitable only in a concurrent
environment, and with some (bad) luck. I only found the bug when studying
the code. So that's why I tried to provide a solid explanation, believing
the fix I'm about to make should be obvious and mergable without worries.
--
Ticket URL: <https://code.djangoproject.com/ticket/22967>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knaperek):
* status: new => assigned
* needs_better_patch: => 0
* owner: nobody => knaperek
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:1>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by knaperek):
My fix is available in this
[https://github.com/knaperek/django/tree/ticket_22967 Github branch]
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:2>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knaperek):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:3>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by knaperek):
I've modified the fix to count with the rare DB behavior when a successful
UPDATE returns zero (as discussed in
[https://code.djangoproject.com/ticket/20988 #20988]).
More info in [https://github.com/django/django/pull/2888 Github
discussion].
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:4>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
concurrency race-condition | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by knaperek):
* keywords: => select_on_save concurrency race-condition
* version: 1.6 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:5>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: select_on_save | Needs documentation: 0
concurrency race-condition | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
On the PR Anssi said at least docs and comments should be updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:6>
Django
value
Reporter: knaperek | Owner: knaperek
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) | Resolution:
Keywords: select_on_save | checkin
concurrency race-condition | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
Anssi will commit this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:7>
Django
value
Reporter: knaperek | Owner: knaperek
Component: Database layer | Version: master
Severity: Normal | Triage Stage: Ready for
Keywords: select_on_save | checkin
concurrency race-condition | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c56c42b5c01ed774cfa8e044cd372a984608536c"]:
{{{
#!CommitTicketReference repository=""
revision="c56c42b5c01ed774cfa8e044cd372a984608536c"
Fixed #22967 -- Made Model._do_update consistent
Made _do_update behave more strictly according to its docs,
including a corner case when specific concurent updates are
executed and select_on_save is set.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22967#comment:8>