12
\$\begingroup\$

I am porting an application from django 1.4.5 to django 1.6.1. I found that commit_on_success has been deprecated. Moreover I found specific incompatibilities when calling get_or_create from inside commit_on_success.

atomic is often mentioned as drop-in replacement for commit_on_success, but using atomic alone does not always guaranteed durability, and there appear to be no official solution to that problem.

I have tried writing a replacement for commit_on_success by building on top of atomic and adding back in the durability guarantee. Does this code look like it solves the problem? Are there cases, where this code would fail to provide atomicity and durability?

# -*- coding: utf-8 -*-

try: from django.db.transaction import (Atomic, TransactionManagementError,
                                        get_connection)
except ImportError: from django.db.transaction import commit_on_success
else:
    class DurableAtomic(Atomic):
        def __enter__(self):
            if get_connection(self.using).in_atomic_block:
                raise TransactionManagementError(
                    "Cannot use DurableAtomic inside Atomic")
            super(DurableAtomic, self).__enter__()
    def commit_on_success(using=None):
        if callable(using):
            return DurableAtomic(None, False)(using)
        return DurableAtomic(using, False)

As suggested by Quentin I have written unit tests for the code:

# -*- coding: utf-8 -*-

from django.contrib.auth.models import User
from django.db.transaction import atomic, TransactionManagementError
from core.commit_on_success import commit_on_success
from django.test import TransactionTestCase

def update_user(username):
    User.objects.filter(username=username) \
                .update(email=username + '@example.org')

def is_updated(username):
    return '.org' in User.objects.get(username=username).email

class TestException(Exception):
    pass

class TestValidation(TransactionTestCase):
    def setUp(self):
        User.objects.create_user('a', 'a@example.com', 'aaa')
        User.objects.create_user('b', 'b@example.com', 'bbb')
        User.objects.create_user('c', 'c@example.com', 'ccc')

    # First test nesting of @atomic and @commit_on_success
    # There are four combinations, but nesting atomic inside atomic is
    # outside of scope for this test case.

    # atomic inside commit_on_success is appropriate usage and must work.

    def test_atomic_in_commit_on_success(self):
        @atomic
        def inner():
            update_user('b')

        @commit_on_success
        def outer():
            update_user('a')
            inner()
            update_user('c')

        outer()

        self.assertTrue(is_updated('a'))
        self.assertTrue(is_updated('b'))
        self.assertTrue(is_updated('c'))

    # commit_on_success inside atomic is inappropriate and must raise an
    # exception. Test catching the exception inside as well as outside the
    # outermost block.

    def test_commit_on_success_in_atomic(self):
        @commit_on_success
        def inner():
            update_user('b')

        @atomic
        def outer():
            update_user('a')
            inner()
            update_user('c')

        self.assertRaises(TransactionManagementError, outer)

        self.assertFalse(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertFalse(is_updated('c'))

    def test_commit_on_success_in_atomic_handled(self):
        @commit_on_success
        def inner():
            update_user('b')

        @atomic
        def outer():
            update_user('a')
            self.assertRaises(TransactionManagementError, inner)
            update_user('c')

        outer()

        self.assertTrue(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertTrue(is_updated('c'))

    # commit_on_success inside commit_on_success is inappropriate and must
    # raise an exception. Test catching the exception inside as well as
    # outside the outermost block.

    def test_commit_on_success_in_commit_on_success(self):
        @commit_on_success
        def inner():
            update_user('b')

        @commit_on_success
        def outer():
            update_user('a')
            inner()
            update_user('c')

        self.assertRaises(TransactionManagementError, outer)

        self.assertFalse(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertFalse(is_updated('c'))

    def test_commit_on_success_in_commit_on_success_handled(self):
        @commit_on_success
        def inner():
            update_user('b')

        @commit_on_success
        def outer():
            update_user('a')
            self.assertRaises(TransactionManagementError, inner)
            update_user('c')

        outer()

        self.assertTrue(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertTrue(is_updated('c'))

    # Next test the appropriate nesting with an exception raised at
    # interesting points during execution.

    def test_exception_in_inner(self):
        @atomic
        def inner():
            update_user('b')
            raise TestException

        @commit_on_success
        def outer():
            update_user('a')
            inner()
            update_user('c')

        self.assertRaises(TestException, outer)

        self.assertFalse(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertFalse(is_updated('c'))

    def test_exception_in_inner_handled(self):
        @atomic
        def inner():
            update_user('b')
            raise TestException

        @commit_on_success
        def outer():
            update_user('a')
            self.assertRaises(TestException, inner)
            update_user('c')

        outer()

        self.assertTrue(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertTrue(is_updated('c'))

    def test_exception_in_outer(self):
        @atomic
        def inner():
            update_user('b')

        @commit_on_success
        def outer():
            update_user('a')
            inner()
            update_user('c')
            # Raising an exception at this exact point of execution means it
            # will be impossible to simultaneously provide durability for the
            # inner block and atomicity for the outer block.
            # Had the innermost block asked for durability, we would have
            # raised TransactionManagementError before reaching this point.
            raise TestException

        self.assertRaises(TestException, outer)

        # Innermost block didn't ask for durability, and it didn't get it.
        # Atomicity for the outer block is preserved, in this particular case
        # by rolling back the transaction due to the exception.
        self.assertFalse(is_updated('a'))
        self.assertFalse(is_updated('b'))
        self.assertFalse(is_updated('c'))

    # Finally test using as context handler rather than decorator

    def test_as_context(self):
        with commit_on_success():
            update_user('a')
            with atomic():
                update_user('b')
            update_user('c')

        self.assertTrue(is_updated('a'))
        self.assertTrue(is_updated('b'))
        self.assertTrue(is_updated('c'))
\$\endgroup\$
  • \$\begingroup\$ Thank your for the tests! Can you license your code to something else than the default cc by-sa 3.0? \$\endgroup\$ – Quentin Pradet Sep 25 '14 at 7:31
7
+100
\$\begingroup\$
  1. Yes, this clever code solves your problem : you won't be able to nest commit_on_success or atomic under commit_on_success. You already know the caveats, but I'll repeat them anyway:

    • If you do nest a block under a commit_on_success, your code will consistently fail: easy to fix as long as this code path is executed in your tests.
    • It's always possible to commit manually using connection.commit(), maybe you're using a library that does weird things, say by having its own transaction manager.
    • It's easy to maliciously change DurableAtomic.
  2. Please, please write tests for this, to see that you do get the correct behaviour with and without nesting, with and without exceptions.

  3. Nested atomic blocks map conceptually well with one big transaction for the outer block and savepoints for the nested blocks. I agree with Kevin Christopher Henry in your SO answer about commit_on_success going away: your concept of "durability without atomicity" doesn't make much sense, I would prefer to rewrite the code if possible to avoid relying on the dubious behavior of commit_on_success.

  4. I've indented your try/except to be PEP8-compliant:

    # -*- coding: utf-8 -*-
    
    try:
        from django.db.transaction import (Atomic, TransactionManagementError,
                                           get_connection)
    except ImportError:
        from django.db.transaction import commit_on_success
    else:
        class DurableAtomic(Atomic):
            def __enter__(self):
                if get_connection(self.using).in_atomic_block:
                    raise TransactionManagementError(
                        "Cannot use DurableAtomic inside Atomic")
                super(DurableAtomic, self).__enter__()
    
        def commit_on_success(using=None):
            if callable(using):
                return DurableAtomic(None, False)(using)
            return DurableAtomic(using, False)
    
\$\endgroup\$
  • \$\begingroup\$ You are right, I should definitely write more unit tests for this. Would it be a good idea to amend my question with unit tests? \$\endgroup\$ – kasperd Sep 23 '14 at 17:37
  • \$\begingroup\$ I'm not sure what the policy is about changing questions, but yeah, go for it! Having a small Django app on git or similar could make things easier to test too. But yeah, I'd like to see the tests. \$\endgroup\$ – Quentin Pradet Sep 23 '14 at 21:31
  • \$\begingroup\$ I don't get your third point. Conceptually durability and atomicity can be completely separated. There may not be a use case for durability without atomicity, but the concept could be implemented regardless. However there are very clear use cases for atomicity both with and without the durability guarantee. And I decided only to implement the combination of the two (because that was easier and covered my needs). Nesting of commit_on_success was never a good idea, and my replacement does not even try to support that. \$\endgroup\$ – kasperd Sep 23 '14 at 23:34
  • \$\begingroup\$ You're saying that BEGIN TRANSACTION; UPDATE1; SAVEPOINT S; UPDATE2; ROLLBACK to S; COMMIT; breaks durability because you lost UPDATE2, I'm saying "You asked for it". Nothing else to add. :) \$\endgroup\$ – Quentin Pradet Sep 24 '14 at 15:23
  • \$\begingroup\$ The innermost code using atomic did not perform a ROLLBACK, neither did it ask for one. It would in fact trigger the creation of a savepoint, but rather than rolling back to that savepoint, it would trigger a savepoint_commit. Some other code then later caused a rollback or an abort of the entire transaction. The author of that code wasn't asking for it. He wasn't asking to have his code executed in a context where atomic provide no durability guarantees. \$\endgroup\$ – kasperd Sep 24 '14 at 17:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.