From 271542dad1686c438f658aa6220982495db09797 Mon Sep 17 00:00:00 2001 From: Michael Sanders Date: Wed, 1 Aug 2018 10:52:28 +0100 Subject: [PATCH] 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. --- AUTHORS | 1 + django/db/models/query.py | 9 ++++-- docs/releases/1.11.16.txt | 13 ++++++++ docs/releases/2.0.9.txt | 13 ++++++++ docs/releases/2.1.1.txt | 3 +- docs/releases/index.txt | 2 ++ tests/get_or_create/models.py | 2 +- tests/get_or_create/tests.py | 58 +++++++++++++++++++++++++++++++++++ 8 files changed, 96 insertions(+), 5 deletions(-) create mode 100644 docs/releases/1.11.16.txt create mode 100644 docs/releases/2.0.9.txt diff --git a/AUTHORS b/AUTHORS index 79abc88a73..b16e729706 100644 --- a/AUTHORS +++ b/AUTHORS @@ -576,6 +576,7 @@ answer newbie questions, and generally made Django that much better: michael.mcewan@gmail.com Michael Placentra II Michael Radziej + Michael Sanders Michael Schwarz Michael Sinov Michael Thornhill diff --git a/django/db/models/query.py b/django/db/models/query.py index e023d7749d..c1c9e1ee6b 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -501,7 +501,9 @@ class QuerySet: obj = self.select_for_update().get(**kwargs) except self.model.DoesNotExist: params = self._extract_model_params(defaults, **kwargs) - obj, created = self._create_object_from_params(kwargs, 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(kwargs, params, lock=True) if created: return obj, created for k, v in defaults.items(): @@ -509,7 +511,7 @@ class QuerySet: 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): """ Try to create an object using passed params. Used by get_or_create() and update_or_create(). @@ -521,7 +523,8 @@ class QuerySet: return obj, True except IntegrityError as e: 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 raise e 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/2.0.9.txt b/docs/releases/2.0.9.txt new file mode 100644 index 0000000000..2def94ef73 --- /dev/null +++ b/docs/releases/2.0.9.txt @@ -0,0 +1,13 @@ +========================== +Django 2.0.9 release notes +========================== + +*Expected September 1, 2018* + +Django 2.0.9 fixes a data loss bug in 2.0.8. + +Bugfixes +======== + +* Fixed a race condition in ``QuerySet.update_or_create()`` that could result + in data loss (:ticket:`29499`). diff --git a/docs/releases/2.1.1.txt b/docs/releases/2.1.1.txt index 42fe6edeb3..dd9662118d 100644 --- a/docs/releases/2.1.1.txt +++ b/docs/releases/2.1.1.txt @@ -9,4 +9,5 @@ Django 2.1.1 fixes several bugs in 2.1. 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 3393ed886d..bb488fd63e 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -40,6 +40,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 2.0.9 2.0.8 2.0.7 2.0.6 @@ -55,6 +56,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 4a33a809bb..6510bb9464 100644 --- a/tests/get_or_create/models.py +++ b/tests/get_or_create/models.py @@ -2,7 +2,7 @@ from django.db import models 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 e4647d2ab5..194d4159b6 100644 --- a/tests/get_or_create/tests.py +++ b/tests/get_or_create/tests.py @@ -535,6 +535,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(TransactionTestCase): available_apps = ['get_or_create']