Merge pull request #14 from skip-pay/tda/properly_close_connection_on_precommit_error

Properly close transaction in case of exception in precommit callbacks

Author: xdaniel3

chamber/patch.py

@@ -1,4 +1,6 @@
 import codecs
+import logging
+import sys
 
 from django.db.models import Model
 from django.db.models.fields import Field
@@ -7,6 +9,9 @@
 from chamber.utils import remove_accent
 
 
+log = logging.getLogger(__name__)
+
+
 class OptionsLazy:
 
     def __init__(self, name, klass):
@@ -100,7 +105,12 @@ def atomic_pre_commit_exit(self, exc_type, exc_value, traceback):
             # No exception and no rollback, pre_commit hooks can be performed on top level atomic block exit function
             while connection.run_pre_commit:
                 sids, callable_hash, func = connection.run_pre_commit.pop(0)
-                func()
+                try:
+                    func()
+                except Exception:
+                    log.exception("Raised error during pre_commit")
+                    exc_type, exc_value, traceback = sys.exc_info()
+                    break
     else:
         if connection.savepoint_ids:
             sid = connection.savepoint_ids[-1]

example/dj/apps/test_chamber/tests/transaction.py

@@ -1,9 +1,9 @@
 from django.test import TransactionTestCase
 from django.db import transaction
 
-from django.db.transaction import on_commit
+from django.db.transaction import on_commit, get_connection
 
-from chamber.utils.transaction import UniquePreCommitCallable, in_atomic_block, pre_commit, smart_atomic
+from chamber.utils.transaction import UniquePreCommitCallable, pre_commit, smart_atomic
 
 from test_chamber.models import TestSmartModel
 
@@ -19,6 +19,10 @@ def add_number(numbers_list, number):
     numbers_list.append(number)
 
 
+def raise_exc():
+    raise Exception()
+
+
 class TransactionsTestCase(TransactionTestCase):
 
     def test_pre_commit_without_atomic_should_be_called_immediately(self):
@@ -130,6 +134,23 @@ def pre_commit_fn_c():
 
         assert_equal(data, ['a', 'b', 'c'])
 
+    def test_transaction_is_properly_closed_if_pre_commit_fails(self):
+        numbers_list = []
+
+        with transaction.atomic():
+            on_commit(lambda: add_number(numbers_list, 2))
+            pre_commit(lambda: raise_exc())
+        assert_equal(numbers_list, [])
+
+        # Check that another transaction is functional
+        with transaction.atomic():
+            connection = get_connection()
+            with connection.cursor() as cursor:
+                cursor.execute("SELECT 1")
+            on_commit(lambda: add_number(numbers_list, 2))
+            pre_commit(lambda: add_number(numbers_list, 1))
+        assert_equal(numbers_list, [1, 2])
+
     def test_chamber_atomic_should_ignore_errors(self):
         with assert_raises(RuntimeError):
             with smart_atomic():