Concurrent requests in Django

Question

I have 2 models: Product and Order.

Product has an integer field for stock, whereas Order has a status and a foreign key to Product:

class Product(models.Model):
    name = models.CharField(max_length=30)
    stock = models.PositiveSmallIntegerField(default=1)

class Order(models.Model):
    product = models.ForeignKey('Product')
    DRAFT = 'DR'; INPROGRESS = 'PR'; ABORTED = 'AB'
    STATUS = ((INPROGRESS, 'In progress'),(ABORTED, 'Aborted'),)
    status = models.CharField(max_length = 2, choices = STATUS, default = DRAFT)

My goal is to have the product's stock decrease by one for each new order, and increase by one for each order cancellation. In that purpose, I've overloaded the save method of the Order model as such (inspired by Django: When saving, how can you check if a field has changed?):

from django.db.models import F

class Order(models.Model):
    product = models.ForeignKey('Product')
    status = models.CharField(max_length = 2, choices = STATUS, default = DRAFT)

    EXISTING_STATUS = set([INPROGRESS])

    __original_status = None

    def __init__(self, *args, **kwargs):
        super(Order, self).__init__(*args, **kwargs)
        self.__original_status = self.status

    def save(self, *args, **kwargs):
        old_status = self.__original_status
        new_status = self.status
        has_changed_status = old_status != new_status
        if has_changed_status:
            product = self.product
            if not old_status in Order.EXISTING_STATUS and new_status in Order.EXISTING_STATUS:
                product.stock = F('stock') - 1
                product.save(update_fields=['stock'])
            elif old_status in Order.EXISTING_STATUS and not new_status in Order.EXISTING_STATUS:
                product.stock = F('stock') + 1
                product.save(update_fields=['stock'])
        super(Order, self).save(*args, **kwargs)
        self.__original_status = self.status

Using the RestFramework, I've created 2 views, one for creating new orders, one for cancelling existing orders. Both use a straightforward serializer:

class OrderSimpleSerializer(serializers.ModelSerializer):

    class Meta:
        model = Order
        fields = (
            'id',
            'product',
            'status',
        )
        read_only_fields = (
            'status',
        )

class OrderList(generics.ListCreateAPIView):
    model = Order
    serializer_class = OrderSimpleSerializer

    def pre_save(self, obj):
        super(OrderList,self).pre_save(obj)
        product = obj.product
        if not product.stock > 0:
            raise ConflictWithAnotherRequest("Product is not available anymore.")
        obj.status = Order.INPROGRESS

class OrderAbort(generics.RetrieveUpdateAPIView):
    model = Order
    serializer_class = OrderSimpleSerializer

    def pre_save(self, obj):
        obj.status = Order.ABORTED

Here is how to access these two views:

from myapp.views import *

urlpatterns = patterns('',
    url(r'^order/$', OrderList.as_view(), name='order-list'),
    url(r'^order/(?P<pk>[0-9]+)/abort/$', OrderAbort.as_view(), name='order-abort'),
)

I am using Django 1.6b4, Python 3.3, Rest Framework 2.7.3 and PostgreSQL 9.2.

My problem is that concurrent requests can increase the stock of a product higher than original stock!

Here is the script I use to demonstrate that:

import sys
import urllib.request
import urllib.parse
import json

opener = urllib.request.build_opener(urllib.request.HTTPCookieProcessor)

def create_order():
    url = 'http://127.0.0.1:8000/order/'
    values = {'product':1}
    data  = urllib.parse.urlencode(values).encode('utf-8')
    request = urllib.request.Request(url, data)
    response = opener.open(request)
    return response

def cancel_order(order_id):
    abort_url = 'http://127.0.0.1:8000/order/{}/abort/'.format(order_id)
    values = {'product':1,'_method':'PUT'}
    data  = urllib.parse.urlencode(values).encode('utf-8')
    request = urllib.request.Request(abort_url, data)
    try:
        response = opener.open(request)
    except Exception as e:
        if (e.code != 403):
            print(e)
    else:
        print(response.getcode())

def main():
    response = create_order()
    print(response.getcode())
    data = response.read().decode('utf-8')
    order_id = json.loads(data)['id']
    time.sleep(1)
    for i in range(2):
        p = Process(target=cancel_order, args=[order_id])
        p.start()

if __name__ == '__main__':
    main()

This script gives the following output, for a product with a stock of 1:

201 # means it creates an order for Product, thus decreasing stock from 1 to 0
200 # means it cancels the order for Product, thus increasing stock from 0 to 1
200 # means it cancels the order for Product, thus increasing stock from 1 to 2 (shouldn't happen)

EDIT

I've added a sample project to reproduce the bug: https://github.com/ThinkerR/django-concurrency-demo

Answer 1

Take a look at django-concurrency. It handles concurrent editing using optimistic concurrency control pattern.

Answer 2

I think the problem is not about updating the product count atomically -- the F() expression for Django ORM should handle that correctly. However the combined operation of:

1. Checking order status (does product count need update?)
2. Updating product count
3. Updating order status (to cancelled)

is not an atomic operation. It is possible to have the following sequence of events for two threads A and B (both handling cancel request for the same order):

A: Check order status: new is cancel, differs from previous one
B: Check order status: new is cancel, differs from previous one
A: Update product count atomically from 0 to 1
B: Update product count atomically from 1 to 2
A: Update order status to cancelled
B: Update order status to cancelled

What you need to do is one of the following:

1. Change the whole operation to occur within a transaction. You didn't mention your database or transaction mode setup. Django defaults to autocommit mode where each database operation is commited separately. To change to transactions (probably tied to the HTTP request), see https://docs.djangoproject.com/en/dev/topics/db/transactions/
2. Create a synchronization barrier to prevent both threads from updating the product count. You could do this with an atomic compare-and-swap operation on the database layer, but I'm not sure whether there's a ready F or similar primitive to do this. (The main idea is to change the ordering of order and product data updates so that you first update and test atomically the order status.)
3. Use some other form of synchronization mechanism, using a distributed lock system (you can use Redis and many other systems for this). I think this would be overkill for this case, though.

In summary: Unless you are using HTTP-level transactions for your application already, try setting ATOMIC_REQUESTS = True in your Django configuration file (settings.py).

If you don't or can't do that please note that the alternative methods do not give you order-product pair consistency. Try to think what will happen if the Django server crashes between updates to the product and the order -- only one will be updated. (This is where database transactions are a must where the database engine notices that the client aborted -- due to broken network connection -- and rolls back the transaction.)

Answer 3

As you mention you have a race condition over concurrent request. To get rid of this you should made the operations atomic. What I would do is make orders operations atomic using Redis. Then writing back to regular database whenever I could.

http://redis.io/

EDIT:

After some comments, it seems that the best approach is to include select_for_update(wait=True)