[Django] #29499: Race condition in QuerySet.update_or_create()
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Sanders |
Type: Bug | Status: new
Component: Database | Version: 2.0
layer (models, ORM) |
Severity: Normal | Keywords: race-condition
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I believe that there is a potential race condition in
`QuerySet.update_or_create()`
When initially trying to obtain the object to update using `get()`, the
row is locked using the `select_for_update()` method - this would lock the
object against changes by other processes until the end of the
transaction. However, if the object does not already exist,
`update_or_create()` then calls the `_create_object_from_params()` method
- see code:
{{{#!python
with transaction.atomic(using=self.db):
try:
obj = self.select_for_update().get(**lookup)
except self.model.DoesNotExist:
obj, created = self._create_object_from_params(lookup,
params)
}}}
The `_create_object_from_params()` method looks like this:
{{{#!python
def _create_object_from_params(self, lookup, params):
"""
Try to create an object using passed params. Used by
get_or_create()
and update_or_create().
"""
try:
with transaction.atomic(using=self.db):
params = {k: v() if callable(v) else v for k, v in
params.items()}
obj = self.create(**params)
return obj, True
except IntegrityError as e:
try:
return self.get(**lookup), False
except self.model.DoesNotExist:
pass
raise e
}}}
Here, it initially tries to create the object. However (assuming that the
object has a unique key constraint policed by the DB), if the object has
already been created meanwhile (by a different process) the
`IntegrityError` is caught and the code uses the `get()` method to attempt
to obtain the newly created object. The object is then returned to the
calling `update_or_create()` method to update - however in this case, the
row has not been locked against changes. So, at this point other processes
are free to modify the DB row and those changes might then be overwritten
by the `save()` in `update_or_create()`.
== Suggested Fix ==
This could be fixed by changing the `_create_object_from_params()` method
to take a new parameter to specify whether the object should be locked on
get, i.e.:
{{{#!python
def _create_object_from_params(self, lookup, params, lock=False):
"""
Try to create an object using passed params. Used by
get_or_create()
and update_or_create().
"""
try:
with transaction.atomic(using=self.db):
params = {k: v() if callable(v) else v for k, v in
params.items()}
obj = self.create(**params)
return obj, True
except IntegrityError as e:
try:
if lock:
return self.select_for_update().get(**lookup), False
else:
return self.get(**lookup), False
except self.model.DoesNotExist:
pass
raise e
}}}
The call to `_create_object_from_params()` from `get_or_create()` would
remain the same, but from `update_or_create()` it would change to:
{{{#!python
obj, created = self._create_object_from_params(lookup,
params, lock=True)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
The issue is legitimate and the suggested fix makes sense; if
`update_or_create` enforces `select_for_update()` when a row exists it
should always do so.
Would you be able to submit a PR on Github with some tests. I guess this
could qualify for backports as it's a possible data loss issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:1>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Michael Sanders
Comment:
I am obtaining permission from my employer and then will create a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:2>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Michael Sanders):
Permission from my employer has been obtained, so I will now work on a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:3>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/10252
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:4>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.0 => master
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:5>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Do you think we should backport to 1.11 based on your comment about data
loss?
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:6>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I think it should be pretty safe to backport for the rare cases when this
happens. It's really an edge case but as Michael demonstrated in his test
it can effectively lead to data-losses.
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:7>
Django
-------------------------------------+-------------------------------------
| Sanders
Component: Database layer | Version: master
(models, ORM) |
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"271542dad1686c438f658aa6220982495db09797" 271542da]:
{{{
#!CommitTicketReference repository=""
revision="271542dad1686c438f658aa6220982495db09797"
Fixed #29499 -- Fixed race condition in QuerySet.update_or_create().
A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before
update_or_create()
saved the object. The update by the other process/thread could be lost.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:8>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"221ef69a9b89262456bb7abe0e5a4b2fda4a0695" 221ef69a]:
{{{
#!CommitTicketReference repository=""
revision="221ef69a9b89262456bb7abe0e5a4b2fda4a0695"
[2.1.x] Fixed #29499 -- Fixed race condition in
QuerySet.update_or_create().
A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before
update_or_create()
saved the object. The update by the other process/thread could be lost.
Backport of 271542dad1686c438f658aa6220982495db09797 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:9>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"4441826026c888a479d5262d47fbbd72a689efa8" 44418260]:
{{{
#!CommitTicketReference repository=""
revision="4441826026c888a479d5262d47fbbd72a689efa8"
[2.0.x] Fixed #29499 -- Fixed race condition in
QuerySet.update_or_create().
A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before
update_or_create()
saved the object. The update by the other process/thread could be lost.
Backport of 271542dad1686c438f658aa6220982495db09797 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:10>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"2668418d99b42599536d353705456cf5db718d48" 2668418d]:
{{{
#!CommitTicketReference repository=""
revision="2668418d99b42599536d353705456cf5db718d48"
[1.11.x] Fixed #29499 -- Fixed race condition in
QuerySet.update_or_create().
A race condition happened when the object didn't already exist and
another process/thread created the object before update_or_create()
did and then attempted to update the object, also before
update_or_create()
saved the object. The update by the other process/thread could be lost.
Backport of 271542dad1686c438f658aa6220982495db09797 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:11>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: race-condition | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"8a0b9051878d1f604ea40e9441be1bc86ca97cf2" 8a0b9051]:
{{{
#!CommitTicketReference repository=""
revision="8a0b9051878d1f604ea40e9441be1bc86ca97cf2"
[1.11.x] Refs #29499 -- Skipped QuerySet.update_or_create() test that
fails on MySQL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:12>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: race-condition | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"6cfe00ee438111af38f1e414bd01976e23b39715" 6cfe00e]:
{{{#!CommitTicketReference repository=""
revision="6cfe00ee438111af38f1e414bd01976e23b39715"
Refs #29499 -- Fixed race condition in update_or_create() test.
The usage of time.sleep() could result in the update_or_create() thread
winning
the race to create the row if the backend takes a while to create a new
connection in the main thread.
Relying on threading.Event ensures that the flow of execution is
systematically
yield back and forth between the main thread and the thread in charge of
performing the background update_or_create().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29499#comment:13>