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 Michael Radziej + Michael Sanders Michael Schwarz Michael Thornhill Michal Chruszcz 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'."