summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Sanders <michael.sanders@arm.com>2018-08-01 10:52:28 +0100
committerTim Graham <timograham@gmail.com>2018-08-02 17:28:23 -0400
commit2668418d99b42599536d353705456cf5db718d48 (patch)
treec946240d4ad044f191f11f6f00eda860ff841e11
parent98c77c5a700eed4bd314a40ab70904db0c0adac6 (diff)
downloaddjango-2668418d99b42599536d353705456cf5db718d48.tar.gz
[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
-rw-r--r--AUTHORS1
-rw-r--r--django/db/models/query.py9
-rw-r--r--docs/releases/1.11.16.txt13
-rw-r--r--docs/releases/index.txt1
-rw-r--r--tests/get_or_create/models.py2
-rw-r--r--tests/get_or_create/tests.py58
6 files changed, 80 insertions, 4 deletions
diff --git a/AUTHORS b/AUTHORS
index 82341c3201..4109686bfb 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -543,6 +543,7 @@ answer newbie questions, and generally made Django that much better:
michael.mcewan@gmail.com
Michael Placentra II <someone@michaelplacentra2.net>
Michael Radziej <mir@noris.de>
+ Michael Sanders <m.r.sanders@gmail.com>
Michael Schwarz <michi.schwarz@gmail.com>
Michael Thornhill <michael.thornhill@gmail.com>
Michal Chruszcz <troll@pld-linux.org>
diff --git a/django/db/models/query.py b/django/db/models/query.py
index 379edf3882..cbf35610db 100644
--- a/django/db/models/query.py
+++ b/django/db/models/query.py
@@ -479,7 +479,9 @@ class QuerySet(object):
try:
obj = self.select_for_update().get(**lookup)
except self.model.DoesNotExist:
- obj, created = self._create_object_from_params(lookup, params)
+ # Lock the row so that a concurrent update is blocked until
+ # after update_or_create() has performed its save.
+ obj, created = self._create_object_from_params(lookup, params, lock=True)
if created:
return obj, created
for k, v in six.iteritems(defaults):
@@ -487,7 +489,7 @@ class QuerySet(object):
obj.save(using=self.db)
return obj, False
- def _create_object_from_params(self, lookup, params):
+ def _create_object_from_params(self, lookup, params, lock=False):
"""
Tries to create an object using passed params.
Used by get_or_create and update_or_create
@@ -500,7 +502,8 @@ class QuerySet(object):
except IntegrityError:
exc_info = sys.exc_info()
try:
- return self.get(**lookup), False
+ qs = self.select_for_update() if lock else self
+ return qs.get(**lookup), False
except self.model.DoesNotExist:
pass
six.reraise(*exc_info)
diff --git a/docs/releases/1.11.16.txt b/docs/releases/1.11.16.txt
new file mode 100644
index 0000000000..04335f9439
--- /dev/null
+++ b/docs/releases/1.11.16.txt
@@ -0,0 +1,13 @@
+============================
+Django 1.11.16 release notes
+============================
+
+*Expected September 1, 2018*
+
+Django 1.11.16 fixes a data loss bug in 1.11.15.
+
+Bugfixes
+========
+
+* Fixed a race condition in ``QuerySet.update_or_create()`` that could result
+ in data loss (:ticket:`29499`).
diff --git a/docs/releases/index.txt b/docs/releases/index.txt
index 19201a632d..344420a015 100644
--- a/docs/releases/index.txt
+++ b/docs/releases/index.txt
@@ -26,6 +26,7 @@ versions of the documentation contain the release notes for any later releases.
.. toctree::
:maxdepth: 1
+ 1.11.16
1.11.15
1.11.14
1.11.13
diff --git a/tests/get_or_create/models.py b/tests/get_or_create/models.py
index b5fe534e3a..8103a451c4 100644
--- a/tests/get_or_create/models.py
+++ b/tests/get_or_create/models.py
@@ -6,7 +6,7 @@ from django.utils.encoding import python_2_unicode_compatible
@python_2_unicode_compatible
class Person(models.Model):
- first_name = models.CharField(max_length=100)
+ first_name = models.CharField(max_length=100, unique=True)
last_name = models.CharField(max_length=100)
birthday = models.DateField()
defaults = models.TextField()
diff --git a/tests/get_or_create/tests.py b/tests/get_or_create/tests.py
index 0c8efb6f4e..ad6030dda2 100644
--- a/tests/get_or_create/tests.py
+++ b/tests/get_or_create/tests.py
@@ -509,6 +509,64 @@ class UpdateOrCreateTransactionTests(TransactionTestCase):
self.assertGreater(after_update - before_start, timedelta(seconds=0.5))
self.assertEqual(updated_person.last_name, 'NotLennon')
+ @skipUnlessDBFeature('has_select_for_update')
+ @skipUnlessDBFeature('supports_transactions')
+ def test_creation_in_transaction(self):
+ """
+ Objects are selected and updated in a transaction to avoid race
+ conditions. This test checks the behavior of update_or_create() when
+ the object doesn't already exist, but another thread creates the
+ object before update_or_create() does and then attempts to update the
+ object, also before update_or_create(). It forces update_or_create() to
+ hold the lock in another thread for a relatively long time so that it
+ can update while it holds the lock. The updated field isn't a field in
+ 'defaults', so update_or_create() shouldn't have an effect on it.
+ """
+ lock_status = {'lock_count': 0}
+
+ def birthday_sleep():
+ lock_status['lock_count'] += 1
+ time.sleep(0.5)
+ return date(1940, 10, 10)
+
+ def update_birthday_slowly():
+ try:
+ Person.objects.update_or_create(first_name='John', defaults={'birthday': birthday_sleep})
+ finally:
+ # Avoid leaking connection for Oracle
+ connection.close()
+
+ def lock_wait(expected_lock_count):
+ # timeout after ~0.5 seconds
+ for i in range(20):
+ time.sleep(0.025)
+ if lock_status['lock_count'] == expected_lock_count:
+ return True
+ self.skipTest('Database took too long to lock the row')
+
+ # update_or_create in a separate thread.
+ t = Thread(target=update_birthday_slowly)
+ before_start = datetime.now()
+ t.start()
+ lock_wait(1)
+ # Create object *after* initial attempt by update_or_create to get obj
+ # but before creation attempt.
+ Person.objects.create(first_name='John', last_name='Lennon', birthday=date(1940, 10, 9))
+ lock_wait(2)
+ # At this point, the thread is pausing for 0.5 seconds, so now attempt
+ # to modify object before update_or_create() calls save(). This should
+ # be blocked until after the save().
+ Person.objects.filter(first_name='John').update(last_name='NotLennon')
+ after_update = datetime.now()
+ # Wait for thread to finish
+ t.join()
+ # Check call to update_or_create() succeeded and the subsequent
+ # (blocked) call to update().
+ updated_person = Person.objects.get(first_name='John')
+ self.assertEqual(updated_person.birthday, date(1940, 10, 10)) # set by update_or_create()
+ self.assertEqual(updated_person.last_name, 'NotLennon') # set by update()
+ self.assertGreater(after_update - before_start, timedelta(seconds=1))
+
class InvalidCreateArgumentsTests(SimpleTestCase):
msg = "Invalid field name(s) for model Thing: 'nonexistent'."