diff --git a/common/djangoapps/course_modes/views.py b/common/djangoapps/course_modes/views.py index afc63362ff..531069b2f6 100644 --- a/common/djangoapps/course_modes/views.py +++ b/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. diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 2e7a31d3ed..6adca7a26c 100644 --- a/common/djangoapps/student/views.py +++ b/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. diff --git a/common/djangoapps/util/db.py b/common/djangoapps/util/db.py new file mode 100644 index 0000000000..3e8354fc6a --- /dev/null +++ b/common/djangoapps/util/db.py @@ -0,0 +1,40 @@ +""" +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 diff --git a/common/djangoapps/util/tests/test_db.py b/common/djangoapps/util/tests/test_db.py new file mode 100644 index 0000000000..99687cadae --- /dev/null +++ b/common/djangoapps/util/tests/test_db.py @@ -0,0 +1,101 @@ +"""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)()