Merge pull request #5366 from edx/usman/tnl367-transaction-level-decorator

MySQL transaction isolation level decorator.

common/djangoapps/course_modes/views.py

@@ -17,6 +17,7 @@ from course_modes.models import CourseMode
 from courseware.access import has_access
 from student.models import CourseEnrollment
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from util.db import commit_on_success_with_read_committed
 from xmodule.modulestore.django import modulestore
 
 
@@ -121,6 +122,7 @@ class ChooseModeView(View):
         return render_to_response("course_modes/choose.html", context)
 
     @method_decorator(login_required)
+    @method_decorator(commit_on_success_with_read_committed)
     def post(self, request, course_id):
         """Takes the form submission from the page and parses it.
 

common/djangoapps/student/views.py

@@ -78,6 +78,7 @@ import track.views
 
 from dogapi import dog_stats_api
 
+from util.db import commit_on_success_with_read_committed
 from util.json_request import JsonResponse
 from util.bad_request_rate_limiter import BadRequestRateLimiter
 
@@ -663,6 +664,7 @@ def try_change_enrollment(request):
 
 
 @require_POST
+@commit_on_success_with_read_committed
 def change_enrollment(request, auto_register=False, check_access=True):
     """
     Modify the enrollment status for the logged-in user.

common/djangoapps/util/db.py

+"""
+Utility functions related to databases.
+"""
+from functools import wraps
+
+from django.db import connection, transaction
+
+
+def commit_on_success_with_read_committed(func):  # pylint: disable=invalid-name
+    """
+    Decorator which executes the decorated function inside a transaction with isolation level set to READ COMMITTED.
+
+    If the function returns a response the transaction is committed and if the function raises an exception the
+    transaction is rolled back.
+
+    Raises TransactionManagementError if there are already more than 1 level of transactions open.
+
+    Note: This only works on MySQL.
+    """
+    @wraps(func)
+    def wrapper(*args, **kwargs):  # pylint: disable=missing-docstring
+
+        if connection.vendor == 'mysql':
+            # The isolation level cannot be changed while a transaction is in progress. So we close any existing one.
+            if connection.transaction_state:
+                if len(connection.transaction_state) == 1:
+                    connection.commit()
+                # We can commit all open transactions. But it does not seem like a good idea.
+                elif len(connection.transaction_state) > 1:
+                    raise transaction.TransactionManagementError('Cannot change isolation level. '
+                                                                 'More than 1 level of nested transactions.')
+
+            # This will set the transaction isolation level to READ COMMITTED for the next transaction.
+            cursor = connection.cursor()
+            cursor.execute("SET TRANSACTION ISOLATION LEVEL READ COMMITTED")
+
+        with transaction.commit_on_success():
+            return func(*args, **kwargs)
+
+    return wrapper

common/djangoapps/util/tests/test_db.py

+"""Tests for util.db module."""
+
+import ddt
+import threading
+import time
+import unittest
+
+from django.contrib.auth.models import User
+from django.db import connection, IntegrityError
+from django.db.transaction import commit_on_success, TransactionManagementError
+from django.test import TransactionTestCase
+
+from util.db import commit_on_success_with_read_committed
+
+
+@ddt.ddt
+class TransactionIsolationLevelsTestCase(TransactionTestCase):
+    """
+    Tests the effects of changing transaction isolation level to READ COMMITTED instead of REPEATABLE READ.
+
+    Note: This TestCase only works with MySQL.
+
+    To run it on devstack:
+    1. Add TEST_RUNNER = 'django_nose.NoseTestSuiteRunner' to envs/devstack.py
+    2. Run "./manage.py lms --settings=devstack test util.tests.test_db"
+    """
+
+    @ddt.data(
+        (commit_on_success, IntegrityError, None, True),
+        (commit_on_success_with_read_committed, type(None), False, True),
+    )
+    @ddt.unpack
+    def test_concurrent_requests(self, transaction_decorator, exception_class, created_in_1, created_in_2):
+        """
+        Test that when isolation level is set to READ COMMITTED get_or_create()
+        for the same row in concurrent requests does not raise an IntegrityError.
+        """
+
+        if connection.vendor != 'mysql':
+            raise unittest.SkipTest('Only works on MySQL.')
+
+        class RequestThread(threading.Thread):
+            """ A thread which runs a dummy view."""
+            def __init__(self, delay, **kwargs):
+                super(RequestThread, self).__init__(**kwargs)
+                self.delay = delay
+                self.status = {}
+
+            @transaction_decorator
+            def run(self):
+                """A dummy view."""
+                try:
+                    try:
+                        User.objects.get(username='student', email='student@edx.org')
+                    except User.DoesNotExist:
+                        pass
+                    else:
+                        raise AssertionError('Did not raise User.DoesNotExist.')
+
+                    if self.delay > 0:
+                        time.sleep(self.delay)
+
+                    __, created = User.objects.get_or_create(username='student', email='student@edx.org')
+                except Exception as exception:  # pylint: disable=broad-except
+                    self.status['exception'] = exception
+                else:
+                    self.status['created'] = created
+
+        thread1 = RequestThread(delay=1)
+        thread2 = RequestThread(delay=0)
+
+        thread1.start()
+        thread2.start()
+        thread2.join()
+        thread1.join()
+
+        self.assertIsInstance(thread1.status.get('exception'), exception_class)
+        self.assertEqual(thread1.status.get('created'), created_in_1)
+
+        self.assertIsNone(thread2.status.get('exception'))
+        self.assertEqual(thread2.status.get('created'), created_in_2)
+
+    def test_transaction_nesting(self):
+        """Test that the decorator raises an error if there are already more than 1 levels of nested transactions."""
+
+        if connection.vendor != 'mysql':
+            raise unittest.SkipTest('Only works on MySQL.')
+
+        def do_nothing():
+            """Just return."""
+            return
+
+        commit_on_success_with_read_committed(do_nothing)()
+
+        with commit_on_success():
+            commit_on_success_with_read_committed(do_nothing)()
+
+        with self.assertRaises(TransactionManagementError):
+            with commit_on_success():
+                with commit_on_success():
+                    commit_on_success_with_read_committed(do_nothing)()