Opened 13 months ago

Closed 11 months ago

Last modified 11 months ago

#29499 closed Bug (fixed)

Race condition in QuerySet.update_or_create()

Reported by: Michael Sanders Owned by: Michael Sanders
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: race-condition
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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:

      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:

 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.:

 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:

             obj, created = self._create_object_from_params(lookup, params, lock=True)

Change History (12)

comment:1 Changed 13 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 12 months ago by Michael Sanders

Owner: changed from nobody to Michael Sanders
Status: newassigned

I am obtaining permission from my employer and then will create a PR.

comment:3 Changed 11 months ago by Michael Sanders

Permission from my employer has been obtained, so I will now work on a PR.

comment:4 Changed 11 months ago by Michael Sanders

Has patch: set

comment:5 Changed 11 months ago by Simon Charette

Triage Stage: AcceptedReady for checkin
Version: 2.0master

comment:6 Changed 11 months ago by Tim Graham

Do you think we should backport to 1.11 based on your comment about data loss?

comment:7 Changed 11 months ago 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.

comment:8 Changed 11 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 271542da:

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.

comment:9 Changed 11 months ago by Tim Graham <timograham@…>

In 221ef69a:

[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

comment:10 Changed 11 months ago by Tim Graham <timograham@…>

In 44418260:

[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

comment:11 Changed 11 months ago by Tim Graham <timograham@…>

In 2668418d:

[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

comment:12 Changed 11 months ago by Tim Graham <timograham@…>

In 8a0b9051:

[1.11.x] Refs #29499 -- Skipped QuerySet.update_or_create() test that fails on MySQL.

Note: See TracTickets for help on using tickets.
Back to Top